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 behavior when a 'store' arg and a 'store_const' arg both store to the same dest is undefined and inconsistent #121829

Open
AdamWill opened this issue Jul 16, 2024 · 3 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@AdamWill
Copy link
Contributor

AdamWill commented Jul 16, 2024

Bug report

Bug description:

import argparse

parser1 = argparse.ArgumentParser()
parser1.add_argument("--squashfs-only", action="store_const", const="squashfs", dest="rootfs_type")
parser1.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs")
print(parser1.parse_args())
parser2 = argparse.ArgumentParser()
parser2.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs")
parser2.add_argument("--squashfs-only", action="store_const", const="squashfs", dest="rootfs_type")
print(parser2.parse_args())

Note the script adds the same two arguments to each parser, but in both possible orders. Output for me, with Python 3.11 and Python 3.13:

Namespace(rootfs_type=None)
Namespace(rootfs_type='squashfs')

AFAICS nothing in argparse docs indicates what the intended behavior in this case is, and it seems surprising and possibly not optimal that the actual behavior differs depending on what order the args are specified in.

This is a real-world issue - because of this, weldr/lorax@01dd27a broke lorax (the Fedora Linux distribution's script for building installer images). In that case, the expected behavior was that the value should be "squashfs" if neither arg was specified on the command line, but because the args were added in the parser1 order (--squashfs-only first), it is actually None.

CPython versions tested on:

3.11, 3.13

Operating systems tested on:

Linux

@AdamWill AdamWill added the type-bug An unexpected behavior, bug, or error label Jul 16, 2024
@AdamWill
Copy link
Contributor Author

In fixing this, I found it's weirdly hard to tell argparse "accept this old argument so people's scripts don't break, but do nothing at all with it". There is no "null" action, and making your own seems weirdly difficult (I tried it for an hour and gave up). In the end I went with a PR that simply keeps --squashfs-only as a store_true and ignores it, but that seems inelegant.

@serhiy-storchaka
Copy link
Member

The code sets the default value from the first action. I think this should not be surprising. The default value for the "--squashfs-only" option was None, and adding the "--rootfs-type" have not changed this.

You can see the same behavior for two "store" actions with different defaults:

import argparse

parser1 = argparse.ArgumentParser()
parser1.add_argument("--foo", action="store", default="foo", dest="dest")
parser1.add_argument("--bar", action="store", default="bar", dest="dest")
print(parser1.parse_args())
parser2 = argparse.ArgumentParser()
parser2.add_argument("--bar", action="store", default="bar", dest="dest")
parser2.add_argument("--foo", action="store", default="foo", dest="dest")
print(parser2.parse_args())

Output:

Namespace(dest='foo')
Namespace(dest='bar')

The conflict between different defaults should be solved in some way, and choosing the first one is not worse than any other way. Changing this now will break existing code which relies on this.

In fixing this, I found it's weirdly hard to tell argparse "accept this old argument so people's scripts don't break, but do nothing at all with it". There is no "null" action, and making your own seems weirdly difficult (I tried it for an hour and gave up). In the end I went with a PR that simply keeps --squashfs-only as a store_true and ignores it, but that seems inelegant.

I do not that "null" action is what you need here. --rootfs-type=erofs --squashfs-only should be equivalent to --rootfs-type=erofs --rootfs-type=squashfs, i.e. set rootfs_type to "squashfs", not to "erofs".

There are a number of options to solve your problem. You can change the order of add_argument(). You can add explicit default for "--squashfs-only". You can use set_defaults() to set defaults instead of specifying them in add_argument().

@AdamWill
Copy link
Contributor Author

I do not that "null" action is what you need here. --rootfs-type=erofs --squashfs-only should be equivalent to --rootfs-type=erofs --rootfs-type=squashfs, i.e. set rootfs_type to "squashfs", not to "erofs".

That's kinda getting into the weeds, but as it happens in this case we actually want --rootfs-type to win. Anyhow, we've solved it downstream already. I figured it was worth mentioning upstream, but I guess your explanation makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
Status: No status
Development

No branches or pull requests

2 participants