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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

argparse BooleanOptionalAction displays default=SUPPRESS unlike other action types #88753

Closed
abadger mannequin opened this issue Jul 9, 2021 · 4 comments · Fixed by #27808
Closed

argparse BooleanOptionalAction displays default=SUPPRESS unlike other action types #88753

abadger mannequin opened this issue Jul 9, 2021 · 4 comments · Fixed by #27808
Labels
3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@abadger
Copy link
Mannequin

abadger mannequin commented Jul 9, 2021

BPO 44587
Nosy @rhettinger, @abadger, @ambv, @mhils
PRs
  • bpo-38956: don't print BooleanOptionalAction's default twice #27672
  • gh-88753: Make BooleanOptionalAction's addition of default to help more similar to other actions #27808
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2021-07-09.05:07:37.449>
    labels = ['type-bug', 'library', '3.11']
    title = 'argparse BooleanOptionalAction displays default=SUPPRESS unlike other action types'
    updated_at = <Date 2021-09-03.01:01:18.075>
    user = 'https://github.com/abadger'

    bugs.python.org fields:

    activity = <Date 2021-09-03.01:01:18.075>
    actor = 'paul.j3'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2021-07-09.05:07:37.449>
    creator = 'a.badger'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44587
    keywords = ['patch']
    message_count = 4.0
    messages = ['397184', '399730', '399731', '399822']
    nosy_count = 5.0
    nosy_names = ['rhettinger', 'a.badger', 'lukasz.langa', 'paul.j3', 'mhils']
    pr_nums = ['27672', '27808']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue44587'
    versions = ['Python 3.11']

    @abadger
    Copy link
    Mannequin Author

    abadger mannequin commented Jul 9, 2021

    This is related to https://bugs.python.org/issue38956 but a different symptom (and the current proposed fix for 38956 will not fix this. My proposed fixes for this would also fix 38956).

    I have the following code which uses BooleanOptionalAction along with a default of SUPPRESS (I use SUPPRESS because I merge these with settings from config and environment variables and then validate and add default values using a schema. SUPPRESS allows me to tell when a value was not specified at the command line by the user):

        whole_site_parser.add_argument('--indexes',    
                                       dest='indexes', action=BooleanOptionalAction,    
                                       default=argparse.SUPPRESS,    
                                       help='Test')

    That code outputs:

    --indexes, --no-indexes
    Test (default: ==SUPPRESS==)

    Similar code that does not use BooleanOptionalAction does not show default: ==SUPPRESS == even when formatter_class=ArgumentDefaultsHelpFormatter is used.

    Looking at the code, this is occurring because BooleanOptionalArgument has its own code to add default: on its own (instead of leaving formatting as the responsibility of the formatter_class). The code in BooleanOptionalArgument handles less cases where the default should be skipped than the ArgumentDefaultsHelpFormatter code; SUPPRESS is one of the missing cases.

    I can see two ways of fixing this:

    (1) Remove the code from BooleanOptionalArgument that adds the default values. It seems to violate the architecture of argparse which delegates modifications to the help message to the formatter_class so this is a special case which could cause issues for future modifications as well.

    (2) If the usefulness of outputting the default values without changing the formatter_class is deemed too important to relinquish, then moving the code that ArgumentDefaultsHelpFormatter uses to determine when to skip adding default to the help message can be extracted from ArgumentDefaultsHelpFormatter and called by both ArgumentDefaultsHelpFormatter and BooleanOptionalArgument .

    In a review of a fix for https://github.com/python/cpython/pull/17447/files#r429630944 raymond hettinger thought that outputting of the default values was important to keep although I'm not clear on whether he considered that the usefulness comes at the price of possibly violating argparse's architecture. If he hasn't changed his mind, then #2 is probably the way to resolve this.

    I can submit a PR for either of these once I know which direction to take (the first is just removing a few lines of code and I've already written the second one since it seemed like the direction that raymond had last asked for).

    Please let me know how you'd like me to proceed.

    @abadger abadger mannequin added 3.9 only security fixes 3.10 only security fixes labels Jul 9, 2021
    @jacobtylerwalls jacobtylerwalls mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 9, 2021
    @ned-deily ned-deily changed the title BooleanOptionalAction displays default=SUPPRESS unlike other action types argparse BooleanOptionalAction displays default=SUPPRESS unlike other action types Jul 10, 2021
    @ned-deily ned-deily changed the title BooleanOptionalAction displays default=SUPPRESS unlike other action types argparse BooleanOptionalAction displays default=SUPPRESS unlike other action types Jul 10, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Aug 17, 2021

    Toshio, as I commented on #71859, that PR 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 this issue to further improve the situation while retaining displaying the default value (option 2 described above) is interesting. If you prepare a PR we can work with, I'll look into merging that as well.

    @ambv
    Copy link
    Contributor

    ambv commented Aug 17, 2021

    Oh, I forgot to say this explicitly: an important reason why the trivial fix was desirable is that it was a clear-cut bugfix, making it easy to backport all the way to the 3.9 branch where BooleanOptionalAction was introduced.

    Having this in place, your potential PR here might be a cleaner refactor without having to worry about backporting. Of course, if we manage to keep it small and isolated, we can bring it to 3.9 and 3.10 as well.

    @ambv ambv added 3.11 only security fixes and removed 3.9 only security fixes 3.10 only security fixes labels Aug 17, 2021
    @abadger
    Copy link
    Mannequin Author

    abadger mannequin commented Aug 18, 2021

    PR Opened.

    A fix for this should be backported as well. However, if you decide you don't want the refactor backported, you can merely continue to change the condition inside of BooleanOptionalAction to repeat all of the same checks as are contained in the older versions' ArgumentDefaultsHelpFormatter.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    ambv pushed a commit that referenced this issue May 3, 2022
    …re similar to other actions (#27808)
    
    Help for other actions omit the default value if default is SUPPRESS or
    already contains the special format string '%(default)'.  Add those
    special cases to BooleanOptionalAction's help formatting too.
    
    Fixes https://bugs.python.org/issue44587 so that default=SUPPRESS is not
    emitted.
    
    Fixes https://bugs.python.org/issue38956 as this code will detect
    whether '%(default)s' has already been specified in the help string.
    
    Signed-off-by: Micky Yun Chan (michiboo): <chanmickyyun@gmail.com>
    Co-authored-by: Micky Yun Chan <michan@redhat.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    1 participant