Allow env vars to override config variables, but not flags

[jacobmischka]

Parses env configurations into matches (for those that can be configured
via matches) instead of relying on dedicated env checks everywhere.

Fixes #1152

[jacobmischka]

Whoops, didn't mean to change that input file, lol.

[jacobmischka]

Oh wait, I didn't realize Clap::Arg has an env command, that's way better.

[eth-p]

I was worried we might have to resort to using two different instances of clap to achieve this. This is a much more maintainable and elegant solution, great work!

I'm currently on mobile so I can't give this a proper review, but if sharkdp or keith-hall don't beat me to it, I'll give it a look through in the morning.

[eth-p]

Sorry for the delay. I've been a bit busy with university the past couple days, and haven't had the time to do any non-trivial reviews until today.

Great work on the pull request! Not only did this fix the issue, it removed some code that wasn't exactly easy to understand at a glance. Before I go ahead and merge this, I have two small requests:

• Your changes use tabs for indentation, while the rest of the project uses spaces.
  A quick cargo fmt should fix that.

• If you could add an entry to the CHANGELOG.md file, I would appreciate it.
  The format we use is as follows:

  - Description of what has been changed, see #123 (@user)
  

[sharkdp]

Also, a quick question: this is a breaking change of some sorts (for bat as command line tool, not bat-as-a-library), correct? If so, it should probably be marked as such in the CHANGELOG.

[eth-p]

It should, but be aware that there is a bug in cargo fmt that breaks some code in src/controller.rs

Well, that's not ideal. It doesn't seem to do that on my installation (latest stable), but I'll keep an eye out for it whenever I suggest running cargo fmt, thanks.

[sharkdp]

Oh dear.. I had an old from August 2019 version of rustfmt in $HOME/.cargo/bin which was overwriting the "system" rustfmt from the current stable toolchain. The bug was fixed in September 2019 (rust-lang/rustfmt#3759) 😄. Thank you for the hint.

I just pushed a commit where I ran cargo fmt which fixed the style in 4-5 places. I do this from time to time but don't really want to enforce it in CI, as it often puts off contributors. I guess we could enable a formatting check if we add a CONTRIBUTING file or a PR template.

[jacobmischka]

Sorry about the formatting changes, my editor is supposedly configured to run rustfmt on save, so not sure why that didn't work correctly.

Thanks!

[eth-p]

Thanks!

For whatever reason, cargo fmt didn't fix the spaces/tabs inconsistency. I didn't want to bug you about it again, so I ended up just fixing it manually (even my cargo fmt didn't work) and force pushing it to your branch. Just waiting on the CI checks to pass now.

---

@sharkdp:

I do this from time to time but don't really want to enforce it in CI, as it often puts off contributors. I guess we could enable a formatting check if we add a CONTRIBUTING file or a PR template.

Oh, that's fine. I think the forced checks are usually unnecessary, but I brought it up this time because mixed spaces/tabs is the bane of editors.

[sharkdp]

@jacobmischka Thank you for this contribution.

As far as I can tell, this does not fix #1152 so far, right? It's certainly a nice code simplification for the current behavior, but I'm not able to override config settings with environment variables.

To reproduce:

Add this to $(bat --config-file):

--pager="echo from-config"

Then, call:

BAT_PAGER="echo from-env" bat <(echo test)

Also, this change modifies the -h/--help text. It adds a [env: …] suffix which is not really self-explanatory, IMO:

        --tabs <T>
            Set the tab width to T spaces. [env: BAT_TABS=]

[jacobmischka]

Oh apologies, I thought I added an integration test for that but obviously I didn't. I'll look into it tonight hopefully and make sure it does.

As for the help text, I think that can be fixed using the hide_env_values() method, though I'm not positive since the docs for that seem to be a little broken right now. I'll also look into this as soon as I can.

Thanks!

[sharkdp]

Sure, no hurry!

I am afraid that the actual solution to this will be quite a bit more involved :-/

[jacobmischka]

Well, you weren't kidding. I admit this solution is a bit filthy, but hey all the test pass 😄! I wouldn't blame you if you had issues with mucking about with clap internals like this, but the previous Vec manipulation was slightly gross as it was already, ha.

Also, I believe hide_env_values() is supposed to solve this problem, but it seems to not be working in all cases (though it is in some, like pager?). I think that's a bug in clap, which I think I'll look into next.

[jacobmischka]

Ah, turns out hide_env_values hides the values from the help text, not the env keys themselves. I think I will look into a PR to clap that adds a flag to hide the keys as well.

[sharkdp]

@jacobmischka Thank you for the updates and the upstream PR. Let's come back to this as soon as your change has made it into a clap release.

[sharkdp]

One year later and still no clap release. We could think about depending on a clap 3 beta release.

[Enselic]

clap 3.0.0 has been released now 🎉 https://github.com/clap-rs/clap/releases/tag/v3.0.0

[jacobmischka]

Unfortunately, clap 3 also breaks the method I was using to modify the arguments, they made that property private to the crate now. I updated this fork, but haven't found a better way to modify the matches. It's possible that there's a better, more supported way in clap 3 that I don't know about, I haven't looked super closely. But regardless, this needs work. I may return to it in the future, but certainly suggestions/help welcome.

[Enselic]

Closing to keep PR inbox clean. I added a note in #1152 that anyone looking to fix this problem to take a look at this PR. Of course feel free to re-open this PR if work resumes on it.