bpo-38956: don't print BooleanOptionalAction's default twice

[mhils]

This PR is a less intrusive fix to not double-print the default value when using argparse.BooleanOptionalAction with ArgumentDefaultsHelpFormatter. The original bug report (bpo-38956) was about removing the default entirely, but @rhettinger made a sensible point in #17447 (comment) that a default should always been shown here. While #17447 seems to work as intended, progress stalled a bit presumably because it's not entirely trivial to review. This PR proposes a more straightforward attempt to solve the problem, which can hopefully be reviewed more easily. If the maintainers ultimately prefers #17447 that is perfectly fine with me of course. 😃

In short, in this PR we make use of the fact that ArgumentDefaultsHelpFormatter already checks if the default is already included:

cpython/Lib/argparse.py

Line 693 in 2b496e7

if '%(default)' not in action.help:

This condition previously did not trigger because BooleanOptionalAction already did the string interpolation directly.

https://bugs.python.org/issue38956

[ambv]

Confirmed that the test fails without the source change:

-   --taz, --no-taz  Whether to taz it (default: True)
+   --taz, --no-taz  Whether to taz it (default: True) (default: True)
?                                                ++++++++++++++++
    --quux QUUX      Set the quux (default: 42)

(that diffing algorithm is weird)

[miss-islington]

Thanks @mhils for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

[miss-islington]

Sorry, @mhils and @ambv, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 1512bc21d60f098a9e9f37b44a2f6a9b49a3fd4f 3.9

[bedevere-bot]

GH-27787 is a backport of this pull request to the 3.10 branch.

[bedevere-bot]

GH-27788 is a backport of this pull request to the 3.9 branch.

[abadger]

Hi, I don't think this was the right fix for this issue (or maybe better stated as... this is an incomplete fix that leaves two diverging code paths which should be consolidated). Please see https://bugs.python.org/issue44587 for my explanation of what I think should be done (Option 2 if we want to go the nonintrusive route). I have an implementation of option 2 vendored into one of my projects here: ansible-community/antsibull-build@67f6d1a#diff-f515d1b9734c55a77e0a94b093b41eb1102a986be60631df39ac1513895aba07 I can pull that out into a pr but I had been waiting on someone to confirm that Option 2 is really the desired outcome considering the architecture of argparse.

[mhils]

Thanks @ambv! ❤️

@abadger: My intention with this PR was to unblock a somewhat stalled issue with changes that are trivial to review. bpo-44587 wasn't linked at bpo-38956 so I wasn't aware of it. The changes here shouldn't affect the usefulness of what you proposed, I'd suggest you go ahead and open a PR for your option 2. :)

[abadger]

<nod> i wanted to point it out because this change doesn't fix the rest of the problems caused by the root issue. For instance, as reported in my issue, SUPPRESS will still be displayed as a default value.

…

On Mon, Aug 16, 2021, 4:09 PM Maximilian Hils ***@***.***> wrote: Thanks @ambv <https://github.com/ambv>! ❤️ @abadger <https://github.com/abadger>: My intention with this PR was to unblock a somewhat stalled issue with changes that are trivial to review. [bpo-44587](https://bugs.python.org/issue44587) <https://bugs.python.org/issue44587> wasn't linked at [bpo-38956](https://bugs.python.org/issue38956) <https://bugs.python.org/issue38956> so I wasn't aware of it. The changes here shouldn't affect the usefulness of what you proposed, I'd suggest you go ahead and open a PR for your option 2. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABTCWU66X6PZ3SEC7C5VZ3T5GLCFANCNFSM5BZR3Q7A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[ambv]

@abadger, this was merged because it's a rather trivial improvement over the status quo. I intend to also release this fix in 3.9.7 on August 30th. Your idea in BPO-44587 to further improve this while retaining displaying the default value is interesting. If you prepare a PR we can work with, we'll look into merging that as well.