Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes error message and formats command line output #139

Merged
merged 6 commits into from
Sep 17, 2019
Merged

Fixes error message and formats command line output #139

merged 6 commits into from
Sep 17, 2019

Conversation

akhilbojedla
Copy link
Contributor

@akhilbojedla akhilbojedla commented Sep 15, 2019 •
edited
Loading

Description

Fixes #129 and #21 partially

Fixes/Implements: #129 #21

Successful Execution:
image

Failed Execution:
image

New dependencies introduced?

  • go-runewidth - Helps to get rune width of strings thus letting us format console messages
  • color - Helps is generating colorized output to console

Checklist:

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog draft for corresponding release
  • I have update README (If needed)

@gopinath-langote
Copy link
Owner

gopinath-langote commented Sep 15, 2019 •
edited
Loading

Hello @akhilbojedla ,

Thanks for PR.

As library https://github.com/fatih/color is archived on Github and no more maintained by the author, should we use the different library for color?

ex:- https://github.com/logrusorgru/aurora

WDYT?

@gopinath-langote gopinath-langote added Hacktoberfest Support to https://hacktoberfest.digitalocean.com/ dependencies Pull requests that update a dependency file enhancement New feature or request In Progress Someone working on the issue and removed Hacktoberfest Support to https://hacktoberfest.digitalocean.com/ labels Sep 15, 2019
@gopinath-langote gopinath-langote added this to the v2.0.0 milestone Sep 15, 2019
@akhilbojedla
Copy link
Contributor Author

Hello @akhilbojedla ,

Thanks for PR.

As library https://github.com/fatih/color is archived on Github and no more maintained by the author, should we use the different library for color?

ex:- https://github.com/logrusorgru/aurora

WDYT?

Hi @gopinath-langote ,

I didn't knew about aurora library. I've replaced https://github.com/fatih/color with https://github.com/logrusorgru/aurora

Let me know if you we need to change anything else.

gopinath-langote
gopinath-langote reviewed Sep 15, 2019
View reviewed changes
cmd/exec/exec.go Outdated Show resolved Hide resolved
cmd/exec/exec.go Outdated Show resolved Hide resolved
cmd/exec/exec.go Outdated Show resolved Hide resolved
cmd/exec/exec.go Outdated Show resolved Hide resolved
cmd/exec/exec.go Outdated Show resolved Hide resolved
cmd/exec/exec.go Outdated Show resolved Hide resolved
testing/fixtures/execute_cmd_fixtures.go
@@ -51,8 +59,7 @@ commands:
- build: echo building project
`

expectedOutput := `No command 'random' found in config file

expectedOutput := aurora.Red("\nError building exectuion plan. Command \"random\" not found.").Bold().String() + `
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think checking color in test is worth and necessary - I am ok to just test the output and not colors.

WDYT?

Copy link
Contributor Author

@akhilbojedla akhilbojedla Sep 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But just comparing the text wont assert true when one of it is coloured. I can switch off colour on aurora but Have to initialize a non default aurora instance based on a flag which by the current implementation takes a while to implement.

@akhilbojedla
Copy link
Contributor Author

@gopinath-langote I've addressed most of your comments but one. I've replied to the remaining one. Let me know what are your thoughts

gopinath-langote
gopinath-langote approved these changes Sep 17, 2019
View reviewed changes
Copy link
Owner

@gopinath-langote gopinath-langote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @akhilbojedla Thanks for the PR.

LGTM. Merging it.

@gopinath-langote gopinath-langote merged commit c9de5a7 into gopinath-langote:master Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gopinath-langote gopinath-langote gopinath-langote approved these changes

Assignees

@akhilbojedla akhilbojedla

Labels
dependencies Pull requests that update a dependency file enhancement New feature or request In Progress Someone working on the issue
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Fix failed command execution message
2 participants
@akhilbojedla @gopinath-langote