Add --passthru option
#741
Conversation
src/app.rs
Outdated
| @@ -166,6 +166,9 @@ pub fn app() -> App<'static, 'static> { | |||
| .arg(flag("no-ignore-vcs")) | |||
| .arg(flag("null").short("0")) | |||
| .arg(flag("only-matching").short("o")) | |||
| .arg(flag("passthru").alias("passthrough") | |||
| .conflicts_with_all(&["only-matching", "replace"]) | |||
| .overrides_with("count")) | |||
There was a problem hiding this comment.
Hmm I kind of feel like --passthru should conflict with --count, no? They are somewhat mutually incompatible flags. I could be convinced though! Maybe explain this a bit more?
I agree with the other conflicts. Thanks for thinking about that!
There was a problem hiding this comment.
I guess i feel like option conflict errors are pretty heavy-handed and disruptive, and they should only be used if it's likely that the user would be confused by the behaviour otherwise; if not, they should just override each other or be ignored.
In this case, i don't think there's any reason to expect rg --passthru -c to do anything but print the count. And that's consistent with stuff like rg --line-number -c, which feels similarly unambiguous to me.
(There is a difference here in that rg -c --passthru actually disables -c, whereas rg -c --line-number just ignores the --line-number. If we do keep the non-conflicting behaviour, i think i should actually change it so that it just checks !self.is_present("count") when it adds the pattern in args.rs.)
That's my inclination, anyway — like i said, i don't feel super strongly about it. If you aren't feeling it i'll change it obv
There was a problem hiding this comment.
Hmmmm... I am conflicted. But you're right, we already have this problem today with other flags. I'm fine with the behavior in this PR, and adding !self.is_present("count") sounds good.
doc/rg.1.md
Outdated
| --passthru, --passthrough | ||
| : Show both matching and non-matching lines. This is equivalent to adding ^ to | ||
| the list of search patterns. This option overrides --count and cannot be used | ||
| with --only-matching or --replace. |
There was a problem hiding this comment.
Yeah I agree that we shouldn't mention ^. I could see the implementation changing for example to support --replace by implementing this flag differently. (e.g., By not modifying the regex.)
|
Updated |
Fixes #740
I ran into one issue that i didn't consider, which is that
--replacewon't work properly with this option (since it treats zero-width matches the same as any others). That's kind of a shame i think (if for no other reason than it would make the unit tests more accurate), but i suppose people can usesedif they really need that functionality.I made it override with
--countsince it seemed relatively safe and intuitive to me. Don't feel strongly about it though.Also, in hind sight maybe it's not worth mentioning that it's equivalent to adding a
^pattern, since it's just an implementation detail that isn't really useful to anyone. Let me know if you agree and i'll remove it.Some of the changes here might conflict with #728, but only slightly if so.