add syntax highlighting for output #52
Conversation
|
Consider this a draft. Of course we can first discuss and make any necessary changes and then merge :) |
|
It currently looks like this
|
|
With yaml, the output looks nice, but with JSON, it doesn't look that great actually, even though JSON is only a subset of YAML. I mean, the braces and the spaces have some highlighting. I have to check if this is really expected
|
|
And I haven't added any unit or E2E test. I was wondering if we should test it and what to test and how to do it. Should we add just E2E test? Since it's more CLI output related. I can't think of anything related to unit test, but I can see that there are some unit tests around output. We could do that. Any thoughts on testing? |
|
|
Could you explain more about what points 2, 3 and 5 mean? I didn't understand them properly |
2 - Ensure that if the command failed without color, it is failing with color also |
|
What's the benefit of 4 (single line check) if 5 does a complete check? |
4 is working for JSON also, I agree that for YAML is redundant if you will do also 5. |
|
About 2 - Why would the error code change because of this option? Only when highlighting has some issues (error), there will be a non-zero exit code |
It shouldn't - this is reason why need such test (I had such bug 20 years ago and it was really nagging) |
|
Makes sense. I'll add it. But I was just wondering, so, the test strategy is - For every feature flag that's present, test if the usage of any flag causes the exit code to change in an unexpected manner? Doesn't that sound tedious? Or it could be done as some sort of parameterized test, but still that would be a lot of combinations depending on the number of flags and possible error situations This project doesn't have much flags, so all good. Just wondering in general 😅 |
I think that this is question for @itaysk and should be documented somewhere. |
|
Cool. I thought you would have an answer since you suggested the strategy |
|
Another thing is, I have only written code for the root command. I'm yet to understand and try out the "get" sub command. If it needs the color, we need to add for it too (looks like it does need it) |
|
Do we surely need the full output check? I think we can trust the library and just check that the option works - that is, invokes the library, by just checking the first line |
karuppiah7890
force-pushed
the
fix-issue-39
branch
from
88041ed to
e8ae3d4
Compare
I am not sure this why I split it to two different tests. |
karuppiah7890
force-pushed
the
fix-issue-39
branch
from
e8ae3d4 to
7fe8626
Compare
|
I have finished all the stuff for this PR. cc @itaysk |
|
You can review and let me know how it looks and if there are any more changes to be done :) |
|
@karuppiah7890 |
|
@AssafKatz3 cool, I'll check it out! Currently we still don't have support for windows however (no artifacts) but yeah, I'll be sure to check it out |
| return nil | ||
| }, | ||
| } | ||
|
|
||
| func printOutput(cmd *cobra.Command, out []byte) error { |
There was a problem hiding this comment.
instead of passing the entire cmd, can you please pass just cmd.OutOrStdout() as io.writer?
| if err != nil { | ||
| return err | ||
| } | ||
| cmd.Println(highlightedOutput) |
There was a problem hiding this comment.
please rebase and use Print based on #57
|
Apparently the library may not work on Windows - #52 (comment) . I'm not working on this PR now, so I'll close it. I hope new contributors will have an opportunity to work on it and also check support for windows based on #6 , #53 |
|
I'll resume working on this soon |
karuppiah7890
force-pushed
the
fix-issue-39
branch
from
7fe8626 to
e5172fb
Compare
|
I'm gonna be checking about https://github.com/alecthomas/chroma library if that's okay. cc @itaysk |
fixes #39