-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
cat: Allow 'bat' to find existing configuration #7488
Conversation
5d59d87
to
017e47c
Compare
What are the list of |
Personally, either most of them ( |
Can that be defined with an environment variable?
No, we definitely don't want to allow them to be passed through as they are likely to affect formula builds. We could consider copying them to |
Yes,
However this suggestion sounds like a better/more general solution; I'll see if I can remodel the PR around that approach… @MikeMcQuaid Where in the codebase is the environment filtering happening? |
I'd rather the opposite: let's scope it back to just Line 65 in e1f3b0b
ENV["BAT_CONFIG_PATH"] = Homebrew::EnvConfig.bat_config_path in cat.rb (and also in the cask one, if desired).
|
b0f034b
to
ecacafa
Compare
@MikeMcQuaid The problem with that previous approach is it still imposes limitations on |
As mentioned: I'd rather we kept this |
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.
Let's use bat-specific, non-XDG variables. Will consider a follow-up PR that handles XDG correctly across the entire codebase.
Right so it's potentially a more general solution… why is that a bad thing? Where else does
There are more
@MikeMcQuaid How would that differ from this approach? |
General solutions are good when they work generally. In this case it's a general solution that affects a single option on a single command.
I can't tell you off the top of my head what other tools Homebrew calls out to that support
We're not trying to be exhaustive here. To be honest, I'm already starting to question the value of adding this much code to support something only one person has asked for.
It would involve passing through |
@danielbayley any news/thoughts on the above? |
@MikeMcQuaid Sure… Well it seems to me we should either support
I think this is the proper approach. I'll take a look at implementing it… Where specifically are |
8077329
to
91630c2
Compare
Because it works fine for me (and others) without configuration. As we can't agree: passing on this for now, sorry.
|
@MikeMcQuaid But I last pushed the exact changes you requested here:
|
Ok, I'll take a look when I can… @MikeMcQuaid Just as an aside, my motivation here is that I have a homebrew-dotfiles tap as my dotfiles, and then set |
2700e74
to
40250ef
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@MikeMcQuaid Is this not ready to merge? |
Sorry for the delay, thanks for the PR and patience @danielbayley! |
brew style
with your changes locally?brew tests
with your changes locally?Following #6504, setting
HOMEBREW_BAT
to anything enables the use ofbat
in place ofcat
. But because of the environment variable filtering, it is unable to recognise user configuration eitherexport
ed asBAT_*
environment variables, or a configuration file at$XDG_CONFIG_HOME/bat/config
.The simplest solution, as currently implemented in this PR, is to just set
HOMEBREW_BAT
to the path of the configuration file. For exampleexport HOMEBREW_BAT=$XDG_CONFIG_HOME/bat/config
. Although, if Homebrew didn't filterXDG_*_HOME
variables from the environment,bat
would work as is, as well as being be a more general solution.Alternatively, Homebrew could avoid filtering
BAT_*
environment variables.