-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Prioritize CLI arguments over env vars when they conflict #5738
Changes from 2 commits
f8d8424
440fb18
ac005fa
c745738
c7831a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ def initialize(&block) | |
Homebrew.args.instance_eval { undef tap } | ||
@constraints = [] | ||
@conflicts = [] | ||
@switch_sources = {} | ||
@processed_options = [] | ||
@desc_line_length = 43 | ||
@hide_from_man_page = false | ||
|
@@ -58,7 +59,7 @@ def switch(*names, description: nil, env: nil, required_for: nil, depends_on: ni | |
set_constraints(name, required_for: required_for, depends_on: depends_on) | ||
end | ||
|
||
enable_switch(*names) if !env.nil? && !ENV["HOMEBREW_#{env.to_s.upcase}"].nil? | ||
enable_switch(*names, source: :env_var) if !env.nil? && !ENV["HOMEBREW_#{env.to_s.upcase}"].nil? | ||
end | ||
alias switch_option switch | ||
|
||
|
@@ -172,12 +173,19 @@ def hide_from_man_page! | |
|
||
private | ||
|
||
def enable_switch(*names) | ||
def enable_switch(*names, source: :cli_arg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might make the callers more obvious to make this a non-default parameter. Also, perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @BenMusch Thoughts on making this a non-default parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine with me, will push a change in an hour or two |
||
names.each do |name| | ||
@switch_sources[option_to_name(name)] = source | ||
Homebrew.args["#{option_to_name(name)}?"] = true | ||
end | ||
end | ||
|
||
def disable_switch(*names) | ||
names.each do |name| | ||
Homebrew.args.delete_field("#{option_to_name(name)}?") | ||
end | ||
end | ||
|
||
# These are common/global switches accessible throughout Homebrew | ||
def common_switch(name) | ||
Homebrew::CLI::Parser.global_options.fetch(name, name) | ||
|
@@ -225,7 +233,13 @@ def check_conflicts | |
|
||
next if violations.count < 2 | ||
|
||
raise OptionConflictError, violations.map(&method(:name_to_option)) | ||
env_var_options = violations.select do |option| | ||
@switch_sources[option_to_name(option)] == :env_var | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds right. Has this been fixed yet? I should also probably re-write the tests to catch this bug in the future There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not yet! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the tests should ideally catch this failure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, will take care of this today There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, looks like the issue with the test is that stubbing env vars isn't working at all. My tests unfortunately were structured in a way that they passed regardless of that. For example, this fails: allow(ENV).to receive(:[]).with("HOMEBREW_SWITCH_A").and_return("1")
expect(ENV["HOMEBREW_SWITCH_A"]).to eq("1") # actually is nil Any idea how to fix this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found the issue, I had my stubs in the wrong order. Pushing a change now, sorry for the bugs! |
||
end | ||
|
||
select_cli_arg = violations.count - env_var_options.count == 1 | ||
raise OptionConflictError, violations.map(&method(:name_to_option)) unless select_cli_arg | ||
env_var_options.each(&method(:disable_switch)) | ||
end | ||
end | ||
|
||
|
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.
would prefer to extract these env/cli values into constants somewhere, but not sure what the style for that is, or even if a simple boolean would be better here
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.
I'm not sure I understand what you mean? Also I'd maybe go for
from: :env
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.
I think this phrasing is clearer:
Would it be better to extract
:env
and:args
to constants, to avoid having magic values floating around, or even just to change it to a booleanis_env_var?
flag?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.
Sorry, still being stupid here! Can you give some examples of what values you think should maybe be constants?
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.
something like:
Just to avoid having
:env
and:arg
scattered around the code as magic valuesThere 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.
Feels a bit overkill. I think the symbols are fine.
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.
Sounds good. This is all set from my perspective, last thing would just be making sure that y'all are okay with the
brew style
failure, or if it would be better to update https://github.com/Homebrew/brew/pull/5738/files#diff-40e44d1232a302a388e451cc3a0a22f9R186 toexpect(...).to be_switch_a
, etc.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.
@BenMusch Need to have
brew style
be passing I'm afraid, thanks.brew style --fix
may be able to do that for you automatically.