WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@trippinganymess
Copy link

This PR:

  • Corrects Option.init logic to ensure options using flag_value and is_flag=False work as documented, accepting just the flag and using the flag_value as the option's value.
  • Adds comprehensive tests in tests/test_optional_value_bug.py that fail before and pass after this fix, demonstrating the bug and its resolution.
  • Updates code documentation with a new .. versionchanged:: note, and adds a changelog entry summarizing the change.

This restores the intended and documented API behavior, aligning Click’s implementation with user expectations and preventing regressions in downstream projects

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create a completely new file to test one bug fix. All that's needed is probably one new parametrization of an existing test.

Also, the docstrings, comments, and verbosity of the tests is not really helping at all.

:param hidden: hide this option from help outputs.
:param attrs: Other command arguments described in :class:`Parameter`.
.. versionchanged:: 8.3.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version is wrong.

:param attrs: Other command arguments described in :class:`Parameter`.
.. versionchanged:: 8.3.0
Fixed a regression where options with ``flag_value`` and ``is_flag=False``
Copy link
Member

@davidism davidism Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change comment is way too verbose. "Restore optional value behavior when flag_value is set."

CHANGES.rst Outdated
Unreleased

- Don't discard pager arguments by correctly using subprocess.Popen. :issue:`3039` :pr:`3055`
- Fix: Restore documented behavior for options with flag_value and is_flag=False (#3084).
Copy link
Member

@davidism davidism Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow the style in the rest of the changelog. Don't add a tag prefix, use the correct reference, add this to a new section rather than an already released version.

Also, the PR title, commit message, docstring, and this line are all different wordings of the same fix. Be consistent.

@davidism
Copy link
Member

davidism commented Oct 7, 2025

I appreciate that you submitted a fix for this. However, your use of AI really detracted from the quality of the PR. Please avoid using AI, or at least carefully review the results and compare them to the project style before submitting.

The initial PR description is also a wordy mess that AI tends to produce.

"fixes #..." should be in the PR description, not a tag in the title.

You haven't configured git correctly, so your commits aren't linked to your GitHub account.

The commit message, PR title, changelog entry, and docstring changelog entry are all worded differently.

@trippinganymess
Copy link
Author

trippinganymess commented Oct 8, 2025

I will try to be more careful next time, Thank you so much for the review

@davidism
Copy link
Member

davidism commented Oct 8, 2025

This needs to be rebased onto stable instead of main as well.

@trippinganymess trippinganymess changed the base branch from main to stable October 8, 2025 05:26
@trippinganymess
Copy link
Author

trippinganymess commented Oct 8, 2025

Hi, I think I have made all the relevant changes, I am sorry for being negligence before. It was my first contribution to open source I went on chasing for a quick win. please lemme know if there is anything still wrong I will try to make it right.

Thank you!!

@trippinganymess trippinganymess force-pushed the fix-3084 branch 2 times, most recently from bb37a63 to 6fbc2ae Compare October 8, 2025 11:50
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.

2 participants