-
Notifications
You must be signed in to change notification settings - Fork 138
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
Basic diff output #238
base: master
Are you sure you want to change the base?
Basic diff output #238
Conversation
Thanks for the PR! I really like how the output looks. I've gone ahead and marked this as a draft while this is still a WIP, but just let me know if you need anything As for early feedback I think my only concern is that it shouldn't replace the existing output, but should be added as another option. Maybe through a |
Didn't know you can do that ;)
Indeed, the word preview has a meaning. Maybe just |
Sure |
This comment was marked as resolved.
This comment was marked as resolved.
Hmmm, I think I would be all right with that. Just push changes and ping me whenever you want feedback I am planning on pushing for a release over the next few days |
This comment was marked as outdated.
This comment was marked as outdated.
You can click the triangle on the left of "example" in the topic. Later updates differ a little in detail, but largely it's the same. The Wikipedia entry on diff is probably also worth a read.
Probably. I never thought of diffs in this perspective, but it does work in this way (showing only changes, and limited surrounding context). |
@nc7s Oh, didn't notice the ▶ next to the Ok, so I'm testing echo "123\n123\n123\n123\n123\n123\n123\n123\n123\n123\n" | cargo run -- -d '2' '9' If the changes are not in consecutive line, then it works as expected: echo "123\n000\n123\n000\n123\n000\n123\n000\n123\n000\n" | cargo run -- -d '2' '9' I still think that |
bat. Now spelled in the
This is expected behavior of diffs. Recommended read: https://en.wikipedia.org/wiki/Diff Sometimes you won't know where and what the changed lines are if no surrounding context is provided. That's why diffs have contexts - to match surrounding lines. They may otherwise update an unrelated but identical line, which is totally possible for short lines of common words. If you are confident, context radius could be reduced, down to zero. I'm considering an option to set that, but haven't decided on its naming yet. @CosmicHorrorDev: copied over from earlier, maybe covered in noise:
|
This is great as a prototype, excellent work. I have long wanted to implement something like this. Not sure if this is actually possible but it would be great if we could integrate one of the rust pagers as a library so that users don't necessarily need to have e.g. |
My thought then was that it feels unnecessary for every CLI program to implement its own colorization, esp. when the output is in a pretty common format. Feels like bloat. |
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.
Minor nits :)
@@ -20,6 +20,11 @@ pub struct Options { | |||
/// format are likely to change in the future). | |||
pub preview: bool, | |||
|
|||
#[arg(short, long)] |
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.
I think diff is short enough to not warrant a short flag, -d
might be more useful for other things in the future
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.
How about -D
? Even sed doesn't have many flags. (I impl'd this after all ;)
And what do you think about an option specifying context radius? And filename for piped diffing?
path: Option<&Path>, | ||
) -> Result<String> { | ||
let diff = TextDiff::from_lines(old.as_ref(), new.as_ref()); | ||
let path = if let Some(path) = path { |
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.
Should just convert to a lossy string here, not sure its worth erroring out. Having the user see squareish symbols is better than not being able to see a diff at all
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.
Indeed when acting as an end user interface, not so when the output is used as a proper diff. Or I'm overestimating the latter use case?
For me the big thing is that an integrated pager can be more aware of its contents. An external pager is usually very naïve when it comes to searching or even determining terminal width |
That's a valid point, although somewhat more of a philosphical one. Ultimately, the users want to see a pretty diff. If we can make that happen without having them install bat and configure a good looking theme..I think that's a clear win. And if someone really wants to use their own diff program, we can detect that our diff output is being piped and just produce the raw version as you suggest. Addendum: nevertheless, shipping this 'raw' diff mode is still a big win for all the power users. Just wanted to provide some ideas and food for thought. |
Also sorry for taking so long to give feedback. I've been stretched pretty thin lately. I likely won't have time to take a look till the new year |
I'm no expert in this field, but fear that properly setting up an integrated one requires serious work (you gotta know the ins and outs). Plus, we've already seen
No worries ;) we all have a bunch of work at year end.
Diff coloring is almost too easy to not implement, certainly can be made to work. The "do one thing" principle sometimes has a vague boundary…
Yeah, configurable colorization was talked about in #268. So, combined, maybe we can have colorless diffs first, then implement proper colorization later in one go. |
With @CosmicHorrorDev stepping down what's the status on this? I think it's possitive if either noctis or me would have merge permissions. As I said before don't want to wholely maintain but I wouldn't mind reviewing PRs and doing basic fixes at least until another maintainer shows up. |
As proposed in #208 (comment), produce diffs of changes, instead of printing entire files.
This is a rough WIP that copies the
terminal-inline
example ofsimilar
and barely works. I intend to make it produce patches that are actually applicable.example diff (note that coloring is provided by bat)