-
-
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
Color --replace text using the match type #625
Conversation
0007601
to
86ef072
Compare
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 looks pretty good to me! Thanks for doing this. :-)
src/printer.rs
Outdated
fn new(replace: &'r [u8], count: &'r mut usize) -> CountingReplacer<'r> { | ||
CountingReplacer { replace: replace, count: count } | ||
fn new( | ||
replace: &'r [u8], count: &'r mut usize, |
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.
Could you put each parameter on its own line please?
src/printer.rs
Outdated
caps.expand(self.replace, dst); | ||
let end = dst.len(); | ||
if start != end { | ||
self.offsets.push(Offset::new(start, dst.len())); |
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 feel like it would be clearer if this were written as Offset::new(start, end)
.
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. :) I originally didn't have the empty replacement check and forgot to change this to end
when I added it. Thanks.
src/printer.rs
Outdated
if line.last() != Some(&self.eol) { | ||
self.write_eol(); | ||
} | ||
self.write_matched_line(offsets, &line[..], false); |
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 &line
should be sufficient here? I guess you might need &*line
because line
is a cow I think.
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.
Yeah, &*line
is needed.
86ef072
to
b7613c5
Compare
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.
Updated
src/printer.rs
Outdated
caps.expand(self.replace, dst); | ||
let end = dst.len(); | ||
if start != end { | ||
self.offsets.push(Offset::new(start, dst.len())); |
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. :) I originally didn't have the empty replacement check and forgot to change this to end
when I added it. Thanks.
src/printer.rs
Outdated
if line.last() != Some(&self.eol) { | ||
self.write_eol(); | ||
} | ||
self.write_matched_line(offsets, &line[..], false); |
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.
Yeah, &*line
is needed.
Thanks! My first contribution to a rust project. :) |
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.
w00t. Thanks for the fixups!
Closes #521