-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Improves Printer #428
Improves Printer #428
Conversation
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 I'm mostly in favor of the changes here, although I noted a couple places where an additional allocation was added.
If you think you fixed bugs in the printer, then I think we need at least one regression test for each bug.
Finally, I'm not totally sold on adding colors for columns and separators. It seems a bit much, although, I suppose it's not a big deal. See #377.
src/app.rs
Outdated
(for fg and bg) or a text style. A special format, {type}:none, \ | ||
will clear all color settings for {type}.\n\nFor example, the \ | ||
following command will change the match color to magenta and \ | ||
the background color for line numbers to yellow:\n\n\ |
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.
These docs also need to be added to doc/rg.1.md
and the doc/rg.1
file needs to be regenerated. (If it's hard for you to install pandoc, then I can do the regeneration.)
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 will update doc/rg.1.md
. There is only pandoc=1.16.0.2
in my system, so do the regeneration on your on, please.
src/printer.rs
Outdated
// N.B. We can't use `write` here because of borrowing restrictions. | ||
if self.context_separator.is_empty() { | ||
return; | ||
if !self.context_separator.is_empty() { |
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.
Why change the coding style here? I much prefer early returns.
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.
U r the boss.
src/printer.rs
Outdated
if self.context_separator.is_empty() { | ||
return; | ||
if !self.context_separator.is_empty() { | ||
let separator = self.context_separator.clone(); |
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.
The previous code called out the borrowing restriction, but you removed that comment and added a clone
. The whole point of the previous code was to avoid the clone
. ;-)
src/args.rs
Outdated
@@ -731,8 +731,10 @@ impl<'a> ArgMatches<'a> { | |||
let mut specs = vec![ | |||
"path:fg:magenta".parse().unwrap(), | |||
"line:fg:green".parse().unwrap(), | |||
"column:fg:yellow".parse().unwrap(), |
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.
Please see #377, which asks for colors on column numbers. What's the use case? Similarly for separators.
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.
The use case is to improve readability. Color helps humans to orient. See attached image in the description of PR
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.
Right, but if you read #377, then I'm specifically calling into question why humans are reading column numbers, especially when the match itself is colored.
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 don't know =( I wanted to color everything in the output...
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 I'd prefer if you backed out the addition of column
and separator
color styles. Whether we add that or not should really be decided in #377.
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.
But... the pic... prettier... =(
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.
More features means more code that I need to maintain. For example, if we ever added code to auto-detect the best color settings (which seems like something Windows users might appreciate), then the more color settings we have, the harder that code will be to write.
Features need to be decided upon carefully. Once they're added, we can essentially never remove them. Sorry.
src/printer.rs
Outdated
self.write(msg.as_bytes()); | ||
let _ = self.wtr.reset(); | ||
let msg = format!("[Omitted long line with {} replacements]", count); | ||
self.write_colored(msg.as_bytes(), |colors| { colors.matched() }); |
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 the braces in your closure here are superfluous.
src/printer.rs
Outdated
let msg = format!("[Omitted long line with {} matches]", count); | ||
self.write(msg.as_bytes()); | ||
let _ = self.wtr.reset(); | ||
self.write_colored(msg.as_bytes(), |colors| { colors.matched() }); |
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 the braces in your closure here are superfluous.
src/printer.rs
Outdated
self.write_path(path.as_ref()); | ||
let _ = self.wtr.reset(); | ||
fn separator(&mut self, sep: &[u8]) { | ||
self.write_colored(&sep, |colors| { colors.separator() }); |
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 the braces in your closure here are superfluous.
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.
OK, this happens a lot. I'll stop picking on it.
src/printer.rs
Outdated
let _ = self.wtr.set_color(self.colors.matched()); | ||
self.write(&buf[m.start()..m.end()]); | ||
let _ = self.wtr.reset(); | ||
self.write_colored(&buf[m.start()..m.end()], |colors| { colors.matched() }); |
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 the braces in your closure here are superfluous.
} else { | ||
b | ||
} | ||
}).collect(); |
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.
This introduces an additional allocation that wasn't there before, probably as a result of refactoring path_to_str_native
to return a slice of bytes. Maybe it should return a cow instead?
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.
https://doc.rust-lang.org/std/primitive.slice.html#method.to_vec
The above let mut path = path.to_vec();
was that allocation. I replaced for with map. No additional allocations are made by this code.
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.
D'oh. I missed the to_vec
in the old code.
9c5d469
to
8bb6d21
Compare
src/args.rs
Outdated
@@ -731,8 +731,10 @@ impl<'a> ArgMatches<'a> { | |||
let mut specs = vec![ | |||
"path:fg:magenta".parse().unwrap(), | |||
"line:fg:green".parse().unwrap(), | |||
"column:fg:yellow".parse().unwrap(), |
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 don't know =( I wanted to color everything in the output...
} else { | ||
b | ||
} | ||
}).collect(); |
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.
https://doc.rust-lang.org/std/primitive.slice.html#method.to_vec
The above let mut path = path.to_vec();
was that allocation. I replaced for with map. No additional allocations are made by this code.
src/printer.rs
Outdated
self.has_printed = true; | ||
let _ = self.wtr.write_all(&self.context_separator); | ||
let _ = self.wtr.write_all(&[self.eol]); | ||
let separator = self.context_separator.clone(); |
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 thought that possibility to color it overcomes 1 additional allocation?
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 don't think those are our only choices. You should be able to add color without additional allocations. The code just might be a bit more circuitous.
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.
Will you help me? I am a rust newbie)
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.
The problem is that self.separator
borrows self
mutably, so you can't also send in a reference to self.context_separator
. There are a few ways to solve this, but the simplest is to do what the old code did: duplicate the code and inline the call to separator
manually.
c08095d
to
691a468
Compare
I'm not debating whether colors makes things easier to read. ripgrep has colors already, so I obviously see the utility. What I'm trying to ascertain is whether it's worth it to add more colors. I'm trying to do that by asking for use cases for reading column numbers. I don't understand why a human would find a column number to be useful information on a regular basis. Please see the discussion in #377. |
@BurntSushi +regressions |
@BurntSushi is there any unresolved question about this PR? |
Looks good, thank you! |
I added coloring for columns and separators. Also I fixed some bugs, when paths in context were not colored.