-
Notifications
You must be signed in to change notification settings - Fork 37
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
Update diff output to delineate between changed and unchanged files #726
Conversation
Signed-off-by: egibs <[email protected]>
Signed-off-by: egibs <[email protected]>
23d5151
to
ffda68c
Compare
I think the default diff output should act more like the
I'm a little on the fence about Unchanged behaviors, but sometimes the context is helpful within an objective; similarly to how diff(1) or GitHub code reviews show context. Both have allowances for more context if necessary, but it isn't the default. I would aim for terser output by default, and finding ways to make the output easier to understand without adding bulk. If there are only 2 lines of difference between versions of a file, I feel like we should be able to keep the output to 5 lines or less. Anything more than that adds cognitive overhead. |
Signed-off-by: egibs <[email protected]>
Signed-off-by: egibs <[email protected]>
Tightened up the output in We'll now only report:
|
Signed-off-by: egibs <[email protected]>
Signed-off-by: egibs <[email protected]>
Generally, this looks good to me. In testing I do see one weird behavior in the terminal mode, the up/down triangles associated with risk changes don't seem quite consistent, e.g.:
I think what's happening is that strings are being compared, rather than the numeric values, and possibly the comparisons have inverted logic; I would have expected the output to look like:
The following patch applied on top of the branch gives me the second result:
but I'm not sure I understand the symbols correctly. |
Yep, it looks like this is backward. Good catch! |
Signed-off-by: Evan Gibler <[email protected]>
Signed-off-by: egibs <[email protected]>
This meant NONE -> MEDIUM would get a different indicator then LOW -> MEDIUM. Signed-off-by: Steve Beattie <[email protected]>
Thanks, the backward indicators commit addresses part of the issue, but the other part I think is happening is that comparisons are made from the strings values in the riskLevel table, not the numeric scores, so the comparison result is unexpected in some situations. I created egibs#6 with what I think the fix for this is, and hopefully a clearer explanation. Thanks! |
…ings terminal.go: when diffiing, compared based on scores, not string values
Signed-off-by: egibs <[email protected]>
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.
Thanks for fixing up my formatting mistake, will use make lint
going forward.
Everything LGTM, thanks!
Closes: #689
This PR adds better context when rendering diffs across the bubbletea, markdown, simple, and terminal renderers.
For example:
Markdown example (excerpt):
Simple example:
Interactive example: