Cleanups and fixes to command parsing #84
Conversation
Hi-Angel
force-pushed
the
fix-args-handling
branch
from
2f370ec
to
0d2db7f
Compare
|
upd: didn't know there are single-dash options like |
src/cli_args.rs
Outdated
| @@ -234,7 +234,7 @@ fn ignore<T>(_: T) {} | |||
|
|
|||
| fn parse_line_number_style<'a>( | |||
| config: &mut AppConfig, | |||
| value: Option<&'a str>, | |||
| value: Option<&'a String>, | |||
There was a problem hiding this comment.
it feel weird that you have to change this type.
what you could do is change the call site to maybe_line_number_style.map(|s| &s[..])
There was a problem hiding this comment.
That looks like overcomplicating, but I'll do if you really want. I'll note in the commit message that I was asked by the maintainer to do that if you're okay with that, because I personally would not like that.
| true | ||
| } | ||
|
|
||
| fn debug(config: &mut AppConfig, args: &mut Peekable<impl Iterator<Item = String>>) -> bool { |
There was a problem hiding this comment.
I prefer to keep the signatures of all the arguments handlers the same (debug, html, line_numbers...) even if the functions themselves look dull.
There was a problem hiding this comment.
Odd, but you are the boss. Do you want me to leave that as is with the warning of args being unused, or is there any way to silence it? Like a pragma or what analog does Rust have
|
please consider adding a test if you fix a bug to avoid regression. |
Hi-Angel
force-pushed
the
fix-args-handling
branch
from
0d2db7f
to
eec4fea
Compare
There is no reason for config-handling hooks (like color(), html(), etc) to peek at the value that caused them to be called. So call next() before calling the hooks. That allows to simplify the code and remove some args, but we do not do that per mookid's review, who asked to leave arguments of the hooks to be the same even if some of the functions do not actually use arguments.
Hi-Angel
force-pushed
the
fix-args-handling
branch
from
eec4fea
to
050d929
Compare
|
Anyway, done. |
So for example, calling:
diffr --line-numbers --help
was resulting in error before this commit:
unexpected line number style: got '--help', expected aligned|compact
Works now.
The weird call `maybe_line_number_style.map()` is implemented by mookid's review who would not like just changing the parse_line_number_style() argument type.
|
Note that there are two warnings now. I'm not sure I'm a fan of that kind of consistency, but you are the maintainer, so here it is. |
Refactoring aside, this fixes the problem when
--color-numberswas followed by another option, such as for examplediffr --line-numbers --help. Such combination of parameters would cause an error before this PR.