Colorize the output of ruff format --diff #10110
Conversation
| } | ||
|
|
||
| unified_diff.to_writer(&mut *writer)?; |
There was a problem hiding this comment.
We should re-use your new diffing for the jupyter notebook case below. But that's something that you might have planned anyway
| for hunk in unified_diff.iter_hunks() { | ||
| let hunk_str = hunk.to_string(); | ||
| for line in hunk_str.lines() { | ||
| let colored_line = match line.chars().next() { | ||
| Some('+') => line.green(), | ||
| Some('-') => line.red(), | ||
| _ => line.normal(), | ||
| }; | ||
| writeln!(writer, "{colored_line}")?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: We can iterate over the changes in the hunk and use the hunk's display to avoid matching on the first character.
// Individual hunks (section of changes)
for hunk in unified_diff.iter_hunks() {
writeln!(writer, "{}", hunk.header())?;
// individual lines
for change in hunk.iter_changes() {
match change.tag() {
ChangeTag::Equal => write!(writer, " {}", change.value())?,
ChangeTag::Delete => write!(writer, "{}{}", "-".red(), change.value())?,
ChangeTag::Insert => {
write!(writer, "{}{}", "+".green(), change.value())?
}
}
}
}There was a problem hiding this comment.
Oh this only colors the + and - compared to also doing change.value().green() and change.value().red() for the Delete and Insert respectively, or is only coloring the symbols the intended behavior?
| let relativized_path = if let Some(path) = path { | ||
| fs::relativize_path(path) | ||
| } else { | ||
| String::new() | ||
| }; |
There was a problem hiding this comment.
This is not related to your change and might be worth a separate PR but showing the same header for both add and remove doesn't feel that useful... We might want to change this that the caller needs to provide labels.
|
And yes, reading test failures that are diffs of diffs is extremely difficult. I had to play with the code myself to figure out what's wrong |
Yeah the headers were flipped compared to how the built in writer was doing it. Changed that and it passes the original tests it was failing and now it's just failing these two integration tests diff_shows_safe_fixes_by_default: I think it has something to do with the ␊ which means linebreak stuff going on so I'll try to play around with the different types of write, writelin and '\n' (which is how the unextracted writer did it but it didn't pass linting) combinations and try to figure it out. |
|
Interesting about the color change of the summary. That's unexpected. Maybe try again after #10127 merges. It mentions a fix related to bogus newlines |
MichaReiser
force-pushed
the
colorize-format-diff
branch
from
a19784c to
bb2ed44
Compare
|
I didn't see any newline issues after upgrading to the latest insta version. So that seems fixed. I refactored the diff code a bit to avoid passing around
I think this is good for merging. Thanks for working on this |
|
|
Oh yeah it looks much cleaner now, won't lie was worried about how I was going to do that since I knew enough to know it was messy but not enough to know on how to not make it so. Also, any reason not to color the whole line (symbols + change.value()) like in the header instead of just the symbols? (line 238, 239) match change.tag() {
ChangeTag::Equal => write!(f, " {}", change.value())?,
ChangeTag::Delete => write!(f, "{}{}", "-".red(), change.value())?,
// ^.red()
ChangeTag::Insert => write!(f, "{}{}", "+".green(), change.value())?,
// ^.green()
} |
Uhhh no, That's not intentional. Would you be interesting in doing a follow up PR? |
|
Follow up at #10183 |
Co-authored-by: Micha Reiser <[email protected]>
Summary
Adds color to the output of
ruff format --diff. Implemented by extracting the logic inside of towriter() and using the Colored library.Test Plan
Tested manually in development with one of the files until colored output was achieved.
cargo run -p ruff -- format --diff crates/ruff_linter/resources/test/fixtures/pycodestyle/W29.py --no-cacheDoesn't pass the ``cargo test``` format diff tests and I'm not sure if it's because something's wrong with the implementation or if the tests need to be updated. Would like some help in that direction either way.
Related issue #9984