Unify -o long option to be --store-dir #59

2024-08-12T15:28:46Z

mslw commented

2024-08-12 15:28:46 +00:00

(Migrated from github.com)

This changes make_studyvisit_archive to use -o / --store-dir, consistent with the other three scripts. The --output-dir argument is marked as deprecated but remains supported for the time being. Closes #52

Use of both long forms is guarded againist by using a mutually exclusive group. A deprecation warning is issued when --output-dir is used. When the deprecated parameter is removed, both the group and the warning can be done away with.

As a side note, it seems that Python 3.13 will add "deprecated" to argparse's add_argument:
https://docs.python.org/3.13/library/argparse.html#deprecated

This changes `make_studyvisit_archive` to use `-o / --store-dir`, consistent with the other three scripts. The `--output-dir` argument is marked as deprecated but remains supported for the time being. Closes #52 Use of both long forms is guarded againist by using a mutually exclusive group. A deprecation warning is issued when --output-dir is used. When the deprecated parameter is removed, both the group and the warning can be done away with. As a side note, it seems that Python 3.13 will add "deprecated" to argparse's add_argument: https://docs.python.org/3.13/library/argparse.html#deprecated

adswa (Migrated from github.com) reviewed

2024-08-19 06:40:29 +00:00

bin/make_studyvisit_archive

					
				@ -198,8 +204,18 @@ if __name__ == '__main__':

				        "directory with the name '<study-id>_<visit_id>' is used "

            "in the future. Use -o/--store-dir instead."

```suggestion "in the future. Use -o/--store-dir instead." ```

adswa (Migrated from github.com) reviewed

2024-08-19 06:59:46 +00:00

bin/make_studyvisit_archive

					
				@ -186,1 +189,4 @@

				        "--output-dir", metavar="PATH",

				        help="Deprecated, will be removed in the future; "

				        "use -o/--store-dir instead.")

				    p.add_argument(

TIL about https://docs.python.org/3.13/library/argparse.html#argparse.ArgumentParser.add_mutually_exclusive_group - thanks! But I also read in the docs

Changed in version 3.11: Calling add_argument_group() or add_mutually_exclusive_group() on a mutually exclusive group is deprecated. These features were never supported and do not always work correctly. The functions exist on the API by accident through inheritance and will be removed in the future.

(ref https://github.com/python/cpython/pull/30098). Maybe it would be more future-proof to not rely on this function?

TIL about https://docs.python.org/3.13/library/argparse.html#argparse.ArgumentParser.add_mutually_exclusive_group - thanks! But I also read in the docs > Changed in version 3.11: Calling [add_argument_group()](https://docs.python.org/3.13/library/argparse.html#argparse.ArgumentParser.add_argument_group) or [add_mutually_exclusive_group()](https://docs.python.org/3.13/library/argparse.html#argparse.ArgumentParser.add_mutually_exclusive_group) on a mutually exclusive group is deprecated. These features were never supported and do not always work correctly. The functions exist on the API by accident through inheritance and will be removed in the future. (ref https://github.com/python/cpython/pull/30098). Maybe it would be more future-proof to not rely on this function?

mslw (Migrated from github.com) reviewed

2024-08-19 10:15:19 +00:00

bin/make_studyvisit_archive

					
				@ -186,1 +189,4 @@

				        "--output-dir", metavar="PATH",

				        help="Deprecated, will be removed in the future; "

				        "use -o/--store-dir instead.")

				    p.add_argument(

Thanks for pointing that comment out - I missed it, so it's a TIL for me, too.

However, If I'm reading it correctly, it only talks about nested groups ("Calling add_argument_group() or add_mutually_exclusive_group() on a mutually exclusive group is deprecated"). In which case, it should still be fine to call it directly on the ArgumentParser.

That being said, I have considered a simpler solution, which would be:

p.add_argument("-o", "--store-dir", "--output-dir")

It would assign to args.store_dir regardless of which long option was used. It's much simpler than the mutually exclusive group, but I could not quickly find a way of introspection (which long option was used) allowing me to raise a deprecation error. It could still at least be noted in the help message.

What would you suggest?

Thanks for pointing that comment out - I missed it, so it's a TIL for me, too. However, If I'm reading it correctly, it only talks about nested groups ("Calling `add_argument_group()` or `add_mutually_exclusive_group()` _on a mutually exclusive group_ is deprecated"). In which case, it should still be fine to call it directly on the `ArgumentParser`. That being said, I have considered a simpler solution, which would be: ``` p.add_argument("-o", "--store-dir", "--output-dir") ``` It would assign to `args.store_dir` regardless of which long option was used. It's much simpler than the mutually exclusive group, but I could not quickly find a way of introspection (which long option was used) allowing me to raise a deprecation error. It could still at least be noted in the help message. What would you suggest?

adswa (Migrated from github.com) reviewed

2024-08-19 11:00:30 +00:00

bin/make_studyvisit_archive

					
				@ -186,1 +189,4 @@

				        "--output-dir", metavar="PATH",

				        help="Deprecated, will be removed in the future; "

				        "use -o/--store-dir instead.")

				    p.add_argument(

ha, thanks for helping me parse this correctly - I read it wrong. In that case, sorry for the noise, please disregard the comment. I like the exclusive group!