Closed AdamWill closed 2 months ago
Here's the test script I used to investigate various options here:
import argparse
class NullAction(argparse.Action):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs, nargs=0, default=None, required=False)
def __call__(self, *args, **kwargs):
pass
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("squashfs-only first, empty args:")
print(parser1.parse_args())
print("squashfs-only first, --squashfs-only passed:")
print(parser1.parse_args(["--squashfs-only"]))
print("squashfs-only first, --rootfs-type=foo passed:")
print(parser1.parse_args(["--rootfs-type=foo"]))
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("rootfs-type first, empty args:")
print(parser2.parse_args())
print("rootfs-type first, --squashfs-only passed:")
print(parser2.parse_args(["--squashfs-only"]))
print("rootfs-type first, --rootfs-type=foo passed:")
print(parser2.parse_args(["--rootfs-type=foo"]))
parser3 = argparse.ArgumentParser()
parser3.add_argument("--squashfs-only", action=NullAction)
parser3.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs")
print("null squashfs-only, empty args:")
print(parser3.parse_args())
print("null squashfs-only, --squashfs-only passed:")
print(parser3.parse_args(["--squashfs-only"]))
print("null squashfs-only, --rootfs-type=foo passed:")
print(parser3.parse_args(["--rootfs-type=foo"]))
parser4 = argparse.ArgumentParser()
parser4.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs")
print("no squashfs-only, empty args:")
print(parser4.parse_args())
print("no squashfs-only, --squashfs-only passed:")
print(parser4.parse_args(["--squashfs-only"]))
print("no squashfs-only, --rootfs-type=foo passed:")
print(parser4.parse_args(["--rootfs-type=foo"]))
You can see what each possibility I tried out does.
Note, the documentation should also have been updated for the addition of --rootfs-type
, it was not. I would roll that into this PR but I'm not sure if the HTML docs are generated somehow (if so, I don't see how) or hand-written.
tweaked to turn it back into a store_true
and simply never use the value, as my attempt to invent a "null" action wasn't really working as expected (it still meant we had an args.squashfs_only
, it was just always None
). Since it doesn't work, there's no point carrying the complexity.
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
src/pylorax/cmdline.py | 0 | 2 | 0.0% | ||
<!-- | Total: | 0 | 2 | 0.0% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
src/pylorax/cmdline.py | 2 | 0.0% | ||
<!-- | Total: | 2 | --> |
Totals | |
---|---|
Change from base Build 9947212609: | 0.0% |
Covered Lines: | 1634 |
Relevant Lines: | 3559 |
FWIW, I'd say having a default for the previous, store_const
version of the arg equally makes it hard to predict argparse's behaviour. What does it mean for an arg with a store_const
action to have the same constant as its default
? Especially when there's a store
arg with the same dest? It's a weird idea.
On the face of it, it makes the store_const
arg a no-op - it will always want to store the same value to the dest, whether it's specified or not. But then if the user passes a different string to the store
arg...what should happen? Which of the two should win? Does it matter whether the store_const
arg was explicitly passed or not?
01dd27a broke lorax due to some unspecified behavior in argparse when two different arguments write to the same target. It added a --rootfs-type argument with a string value and a default of "squashfs", and changed --squashfs-only to write the value "squashfs" to the same target (rootfs_type) if passed.
Unfortunately, if you do this with --squashfs-only first and --rootfs-type second, then if neither --squashfs-only nor --rootfs-type is passed, the value of rootfs_type in the resulting args dict is None, not "squashfs" as expected.
If you reverse the order of the args - so --rootfs-type is first and --squashfs-only is second - then if neither arg is passed, the value of rootfs_type comes out as "squashfs" as expected. So we could just swap the args in both places. However, this strikes me as fragile as it seems like this behaviour is not defined and could change unexpectedly.
It seems safer, to me, to accept --squashfs-only but simply ignore it (since the default value of --rootfs-type is squashfs anyway, and if someone were to specify both, I think it's sane for --rootfs-type to 'win'). Weirdly, argparse doesn't seem to have a built-in way to specify a completely no-op argument, so let's just keep it as
store_true
like it used to be, and simply never use the value.Alternatively we could use parse_known_args instead of parse_args and then check that the only unknown arg was squashfs-only, but we'd have to do that in multiple places, this seems a bit simpler.