Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: passed arguments should take precedence over values in config #2100

Merged
merged 12 commits into from May 14, 2022

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Dec 19, 2021

Relates to:

issue: #2096
PR: #1992
PR: #2006

Problem

There were a few problems with pull 1992:

  • Positional arguments combined with defaults (solved by 2006)
  • Positional arguments combined with config objects (solved by this PR)
  • Not accounting for kebab case arguments (solved by this PR)

Questions

  • Are there any other sources of values that I'm not accounting for? (I would rather not have another follow up PR for a third source 馃う )
  • Is there a better way to write the logic? It works, but I can't help but think there's a more elegant solution.

BEGIN_COMMIT_OVERRIDE
fix: veriadic arguments override array provided in config (the same as multiple dash arguments).
END_COMMIT_OVERRIDE

@jly36963 jly36963 requested a review from bcoe Dec 19, 2021
Copy link
Member

@bcoe bcoe left a comment

What's the specific breaking change this will expose ... if it's behavior that people would never want, and no tests caught it, then I could imagine us not taking this as a major change -- worst case perhaps we could put this behavior behind a flag, to help people bumping into issues, and then swap the default value in the next major.

@jly36963 jly36963 changed the title fix!: provided values should not be combined with config values fix: provided values should not be combined with config values Dec 20, 2021
@aol-nnov
Copy link

@aol-nnov aol-nnov commented Dec 22, 2021

@bcoe I'm not sure I understand you correctly. Does your comment states this change will not be included until next major release unless it is hidden behid a flag or the other way around - it is for such an uncommon (rare) case that the author should not worry about any breakage?

Copy link
Member

@bcoe bcoe left a comment

Thinking on this one a bit, a couple things jump out at me:

  1. Rather than implicitly deciding whether to concat values, based on whether a default value has been configured for a key, better that we figure out a way to track (during parsing) whether a default value has been used -- we've talked in the past about adding an isDefaulted method, this would be a great feature for users, and would also allow us to simplify this logic.
  2. Providing config feels different than default values, and I could imagine 50% of people wishing that values would be combined, and the other 50% wishing for the opposite behavior. This makes me think that rather than taking this as a breaking change, we'd do better to introduce a parser flag, to allow folks to configure which approach is taken.

@jly36963
Copy link
Contributor Author

@jly36963 jly36963 commented Jan 2, 2022

Questions:

  • I added helpers (eg. isDefaulted). Will they need to be changed to accommodate aliases, or configurations like strip-dashed?
  • Should an option for overwrite-config or combine-config be added, so that the behavior around merging config & user provided arrays can be configured?

@jly36963 jly36963 requested a review from bcoe Mar 26, 2022
@jly36963 jly36963 marked this pull request as ready for review Apr 16, 2022
@bcoe
Copy link
Member

@bcoe bcoe commented Apr 20, 2022

@jly36963 thank you for this great work 馃憦, I will make an effort to review soon.

bcoe
bcoe approved these changes May 14, 2022
@bcoe bcoe changed the title fix: provided values should not be combined with config values fix: passed arguments should take precedence over values in config May 14, 2022
@bcoe bcoe merged commit 4dac5b8 into yargs:main May 14, 2022
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants