Prioritize CLI arguments over env vars when they conflict #5738

BenMusch · 2019-02-15T06:20:15Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally? (Note: Brew style fails, but the failure is there to keep it consistent with other tests in the file. Happy to change it if y'all prefer)
Have you successfully run brew tests with your changes locally?

Addresses an issue mentioned here: #5736

(Caveat: The issue linked also points out a disconnect between the help text of the command and its actual behavior, which should probably be fixed separately)

I agree with that issue that CLI flags should take priority over environment variables when they conflict, and it's behavior I'm used to in other applications. This PR does that by selecting the CLI-passed argument when there are conflicting switches and all but one of the switches are specified via an env var.

Not 100% tied to this being the correct behavior -- just opening this in case y'all agree that it is

Also, I think there's also an opportunity for a richer API to resolve conflicts here, something along the lines of:

def switch(*names, description: nil, env: nil, required_for: nil, depends_on: nil, env_priority: 1, cli_priority: 2)

Then, you could specify the priority of an argument so that it would always take priority over another when they conflict, etc. But that seems like it may be more code than is worth maintaining

BenMusch · 2019-02-15T06:21:43Z

Library/Homebrew/cli_parser.rb

@@ -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?


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

I'm not sure I understand what you mean? Also I'd maybe go for from: :env

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 boolean is_env_var? flag?

Sorry, still being stupid here! Can you give some examples of what values you think should maybe be constants?

something like:

class Parser SWITCH_SOURCE_ENV_VAR = :env SWITCH_SOURCE_CLI_ARG = :arg ... def switch(...) ... enable_switch(*names, source: SWITCH_SOURCE_ENV_VAR) end ... def check_conflicts ... env_var_options = violations.select do |option| @switch_sources[option_to_name(option)] == SWITCH_SOURCE_ENV_VAR end end end

Just to avoid having :env and :arg scattered around the code as magic values

Feels a bit overkill. I think the symbols are fine.

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 to expect(...).to be_switch_a, etc.

@BenMusch Need to have brew style be passing I'm afraid, thanks. brew style --fix may be able to do that for you automatically.

MikeMcQuaid

Nice work on this! I like the direction.

MikeMcQuaid · 2019-02-15T08:58:22Z

Library/Homebrew/cli_parser.rb

@@ -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?


I'm not sure I understand what you mean? Also I'd maybe go for from: :env

MikeMcQuaid · 2019-02-15T08:59:52Z

Library/Homebrew/cli_parser.rb

@@ -172,12 +173,19 @@ def hide_from_man_page!

      private

-      def enable_switch(*names)
+      def enable_switch(*names, source: :cli_arg)


It might make the callers more obvious to make this a non-default parameter. Also, perhaps from: :args

@BenMusch Thoughts on making this a non-default parameter?

Fine with me, will push a change in an hour or two

MikeMcQuaid · 2019-02-15T09:21:58Z

But that seems like it may be more code than is worth maintaining

Agreed.

I agree with that issue that CLI flags should take priority over environment variables when they conflict, and it's behavior I'm used to in other applications.

Also agreed 👍

MikeMcQuaid · 2019-02-16T09:49:33Z

Library/Homebrew/cli_parser.rb

@@ -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)


Yeh, much prefer this, thanks again!

MikeMcQuaid · 2019-02-16T19:15:14Z

Thanks again @BenMusch, great work!

zmwangx · 2019-02-25T09:32:56Z

Library/Homebrew/cli_parser.rb

@@ -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


:env_var should be :env?

Sounds right. Has this been fixed yet? I should also probably re-write the tests to catch this bug in the future

Yeah, the tests should ideally catch this failure

Sounds good, will take care of this today

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?

Found the issue, I had my stubs in the wrong order. Pushing a change now, sorry for the bugs!

BenMusch added 2 commits February 15, 2019 00:52

Prioritize CLI arguments over env vars when they conflict

f8d8424

Fix 1 style conflict, keep the other for consistency

440fb18

BenMusch commented Feb 15, 2019

View reviewed changes

MikeMcQuaid requested changes Feb 15, 2019

View reviewed changes

BenMusch added 2 commits February 15, 2019 11:01

rename source to from, change values

ac005fa

Make from: required, fix flaky test

c745738

MikeMcQuaid reviewed Feb 16, 2019

View reviewed changes

Get brew style to pass

c7831a4

BenMusch force-pushed the handle-conflicts branch from 29d4d77 to c7831a4 Compare February 16, 2019 12:56

MikeMcQuaid approved these changes Feb 16, 2019

View reviewed changes

MikeMcQuaid merged commit f3716fe into Homebrew:master Feb 16, 2019

zmwangx reviewed Feb 25, 2019

View reviewed changes

BenMusch mentioned this pull request Feb 25, 2019

Fix bug in CLI arg/env var prioritization + corresponding tests #5805

Merged

6 tasks

lock bot added the outdated PR was locked due to age label Mar 27, 2019

lock bot locked as resolved and limited conversation to collaborators Mar 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize CLI arguments over env vars when they conflict #5738

Prioritize CLI arguments over env vars when they conflict #5738

BenMusch commented Feb 15, 2019 •

edited

Loading

BenMusch Feb 15, 2019

MikeMcQuaid Feb 15, 2019

BenMusch Feb 15, 2019 •

edited

Loading

MikeMcQuaid Feb 15, 2019

BenMusch Feb 15, 2019

MikeMcQuaid Feb 15, 2019

BenMusch Feb 15, 2019

MikeMcQuaid Feb 16, 2019

MikeMcQuaid left a comment

MikeMcQuaid Feb 15, 2019

MikeMcQuaid Feb 15, 2019

MikeMcQuaid Feb 15, 2019

BenMusch Feb 15, 2019

MikeMcQuaid commented Feb 15, 2019

MikeMcQuaid Feb 16, 2019

MikeMcQuaid commented Feb 16, 2019

zmwangx Feb 25, 2019

BenMusch Feb 25, 2019

MikeMcQuaid Feb 25, 2019

scpeters Feb 25, 2019

BenMusch Feb 25, 2019

BenMusch Feb 25, 2019

BenMusch Feb 25, 2019

Prioritize CLI arguments over env vars when they conflict #5738

Prioritize CLI arguments over env vars when they conflict #5738

Conversation

BenMusch commented Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMusch Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 15, 2019

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenMusch commented Feb 15, 2019 •

edited

Loading

BenMusch Feb 15, 2019 •

edited

Loading