Accept but ignore the old --squashfs-only argument

[AdamWill]

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.

[AdamWill]

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.

[AdamWill]

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.

[AdamWill]

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.

[coveralls]

Pull Request Test Coverage Report for Build 9949378484

Details

• 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage remained the same at 43.457%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/pylorax/cmdline.py	0	2	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/pylorax/cmdline.py	2	0.0%

Totals
Change from base Build 9947212609:	0.0%
Covered Lines:	1634
Relevant Lines:	3559

---

💛 - Coveralls

[bcl]

Thanks! ends up it is a missing default="squashfs" in the lmc parser, but I like this better.

[AdamWill]

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?