inm7/inm-icf-utilities
10
0
Fork 0
Code Issues 6 Pull requests 5 Packages Activity Actions

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

Merged
mslw merged 2 commits from unify-long-opt into main 2024-09-12 13:04:27 +00:00
Conversation 5 Commits 2 Files changed 2 +21 -5
2 changed files with 21 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

2
.appveyor.yml
View file

@ -80,7 +80,7 @@ install:
for s in $STUDIES; do
for v in visit_a visit_b; do
icf-utils make_studyvisit_archive
--output-dir "$STUDIES_DIR"
--store-dir "$STUDIES_DIR"
--id $s $v
"$FROM_SCANNER";
done

24
bin/make_studyvisit_archive
View file

@ -22,6 +22,7 @@ from datetime import datetime
from hashlib import md5
from pathlib import Path
from typing import Dict
import warnings
from tqdm import tqdm
@ -178,11 +179,16 @@ def main(input_base_dir: str,
if __name__ == '__main__':
import argparse
p = argparse.ArgumentParser(description=__doc__)
p.add_argument(
"-o", "--output-dir", metavar='PATH', default=os.getcwd(),
g = p.add_mutually_exclusive_group()
g.add_argument(
"-o", "--store-dir", metavar='PATH', default=os.getcwd(),
help="Base directory to place the archive structure in. "
"The corresponding '<study-id>/' subdirectory for the "
"study is created automatically, if needed")
"study is created automatically, if needed.")
g.add_argument(
"--output-dir", metavar="PATH",
help="Deprecated, will be removed in the future; "
"use -o/--store-dir instead.")
p.add_argument(
adswa commented 2024-08-19 06:59:46 +00:00 (Migrated from github.com)
Copy link

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 commented 2024-08-19 10:15:19 +00:00 (Migrated from github.com)
Copy link

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 commented 2024-08-19 11:00:30 +00:00 (Migrated from github.com)
Copy link

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!

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!
'--id', nargs=2, metavar=('STUDY-ID', 'VISIT-ID'), required=True,
help="The study and visit identifiers, used to name and "
@ -198,8 +204,18 @@ if __name__ == '__main__':
"directory with the name '<study-id>_<visit_id>' is used "
adswa commented 2024-08-19 06:40:29 +00:00 (Migrated from github.com)
Copy link
            "in the future. Use -o/--store-dir instead."
```suggestion "in the future. Use -o/--store-dir instead." ```
"to place all archive content in.")
args = p.parse_args()
if args.output_dir is not None:
store_dir = args.output_dir
msg = (
"--output-dir argument is deprecated and will be removed "
"in the future. Use -o/--store-dir instead."
)
warnings.warn(msg, DeprecationWarning)
else:
store_dir = args.store_dir
main(input_base_dir=args.input_dir,
output_base_dir=args.output_dir,
output_base_dir=store_dir,
study_id=args.id[0],
visit_id=args.id[1],
)