-
-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
Fixes error message and formats command line output #139
Fixes error message and formats command line output #139
Conversation
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. |
@@ -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() + ` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 |
There was a problem hiding this 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.
Description
Fixes #129 and #21 partially
Fixes/Implements: #129 #21
Successful Execution:
Failed Execution:
New dependencies introduced?
Checklist: