Support for ignored-suffix CLI arguments #1892
Conversation
bojan88
force-pushed
the
ignored-suffix-arg
branch
from
c9f781b
to
175faf6
Compare
bojan88
force-pushed
the
ignored-suffix-arg
branch
from
175faf6
to
700ee57
Compare
There was a problem hiding this comment.
@Enselic please take a look again when you have time.
I reverted some public API changes, and I'm passing config around.
From what I understand, we'll need to deprecate a few more functions here and add new versions. I can update that once you give go-ahead, but until then, I'll leave this as a draft.
bojan88
force-pushed
the
ignored-suffix-arg
branch
from
da575f7
to
74405b9
Compare
|
Again, thanks for working on this. I left som high level comments. Another thing that would be great to iterate on early is the CLI for this. Could you add a single test to |
|
Sorry for the delay. I'll convert this to draft until I find time to work on it. @Enselic it would be awesome if you could give me an example where to put files for integration test, and how many. Otherwise I can come up with something, and we can reiterate from that. Thanks, I appreciate yor reviews |
|
This should be ready for review. |
There was a problem hiding this comment.
I skimmed it over again, and it already looks much better! Thanks!
I have a couple of more high level comments.
I expect to get time for a detailed review in a not too distant future. Would be great if you could take care of high level comments in the meantime.
Not at all. I really appreciate your patience and suggestions, and always thinking "I should've seen this on my own". |
There was a problem hiding this comment.
I feel done with reviewing now, because:
- Public API for bat-as-a-library looks good
- CLI looks good
- Regression tests exists
I'm not that picky with the code for the implementation since that can easily be tweaked later as long as regression tests are in place. The only thing that's hard to tweak is the public interfaces, both library-wise and CLI-wise. But again, those look good to me now.
I don't know of a simple way to diff public APIs between commits. But if I use my home-cooked method, which is:
% git checkout master
% RUSTDOCFLAGS='-Z unstable-options --output-format json' cargo +nightly doc --no-deps
% jq '.index | .[] | select(.crate_id == 0) | .name' target/doc/bat.json | sort > /tmp/api-master
% git checkout ignored-suffix-arg
% RUSTDOCFLAGS='-Z unstable-options --output-format json' cargo +nightly doc --no-deps
% jq '.index | .[] | select(.crate_id == 0) | .name' target/doc/bat.json | sort > /tmp/api-new
The only change in the API is the addition of the new function we added:
% diff -u /tmp/api-master /tmp/api-new
--- /tmp/api-master 2021-11-13 14:30:52.544066658 +0100
+++ /tmp/api-new 2021-11-13 14:31:42.940187393 +0100
@@ -142,6 +142,7 @@
"input_stdin"
"InRange"
"insert"
+"insert_ignored_suffix"
"InvalidPagerValueBat"
"Io"
"is_compatible_with"
so everything seems to be in order in that regard.
Approved!
I will leave this open for some time to give other maintainers a chance to review. CC: @sharkdp @keith-hall
|
@sharkdp @keith-hall Not sure if you saw my ping, so just wanted to let you know that I intend to merge this early next week. Let me know if you would like more time to review. See #1892 (review) for my "final words" on this PR. |
|
Looks awesome to me - nice work! |
| .number_of_values(1) | ||
| .multiple(true) | ||
| .takes_value(true) | ||
| .long("ignored-suffix") |
There was a problem hiding this comment.
I think we should consider making this .hidden_short_help(true), such that we only show it on bat --help, but not for bat -h (which should only show the most commonly used options).
There was a problem hiding this comment.
Thanks @sharkdp for explanation.
It's updated.
|
I have confirmed that the most recent commit made Since all active maintainers have approved this PR I see no point in waiting until the beginning of next week to merge. Merging! 🎉 |
Implements #1824