Skip to content

Give Option.show_default higher priority wrt Context.show_default#1964

Merged
davidism merged 1 commit into
pallets:mainfrom
janluke:fix/show_default-priority
Feb 20, 2022
Merged

Give Option.show_default higher priority wrt Context.show_default#1964
davidism merged 1 commit into
pallets:mainfrom
janluke:fix/show_default-priority

Conversation

@janluke
Copy link
Copy Markdown
Collaborator

@janluke janluke commented Jun 18, 2021

Fixes #1963.

After this, a non-null show_default passed to an option overrides the corresponding context setting.

Implementation notes

  • This also fixes the previous type hint of Option.show_default, adding str to the Union.
  • I didn't include str in the type of Context.show_default because, as far as I understand, passing a str makes sense for dynamic options (not for a global default).
  • Added a static constant Option.DEFAULT_SHOW_DEFAULT = False. If you change your mind about the default, the tests I added don't require any change.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@janluke janluke force-pushed the fix/show_default-priority branch from c294519 to 500e81e Compare June 18, 2021 18:20
@janluke janluke force-pushed the fix/show_default-priority branch from 500e81e to 0d58b4e Compare June 26, 2021 11:53
@janluke
Copy link
Copy Markdown
Collaborator Author

janluke commented Jun 26, 2021

@davidism I think Option.DEFAULT_SHOW_DEFAULT is redundant. We could just set Context.show_default = False rather than to None. Let me know what you think.

EDIT: nope, this would break the propagation from parent context.

@davidism davidism added this to the 8.1.0 milestone Jul 4, 2021
@janluke
Copy link
Copy Markdown
Collaborator Author

janluke commented Dec 30, 2021

@davidism Give me a shout when it's time to release 8.1.0 and I'll resolve conflicts.

@davidism davidism force-pushed the fix/show_default-priority branch from 0d58b4e to bcd7b77 Compare February 20, 2022 03:04
@davidism davidism force-pushed the fix/show_default-priority branch from bcd7b77 to 144f370 Compare February 20, 2022 16:47
@davidism davidism merged commit 166b312 into pallets:main Feb 20, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Give Option.show_default higher priority wrt Context.show_default

2 participants