-
-
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 4 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 | ||
|
@@ -51,14 +52,14 @@ def switch(*names, description: nil, env: nil, required_for: nil, depends_on: ni | |
end | ||
process_option(*names, description) | ||
@parser.on(*names, *wrap_option_desc(description)) do | ||
enable_switch(*names) | ||
enable_switch(*names, from: :args) | ||
end | ||
|
||
names.each do |name| | ||
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, from: :env) 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, from:) | ||
names.each do |name| | ||
@switch_sources[option_to_name(name)] = from | ||
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.
Yeh, much prefer this, thanks again!