v7labs / darwin-py

Library and commandline tool for managing datasets on darwin.v7labs.com
MIT License
115 stars 40 forks source link

[DAR-2294][External] Made text a required argument when posting comments through CLI #850

Closed JBWilkie closed 1 month ago

JBWilkie commented 1 month ago

Problem

When posting dataset item comments through the CLI, the text argument should be required. Not providing the text argument with a value causes the API request to post the comment to fail

Solution

Make the text argument required instead

Changelog

Made the comment text argument required when posting dataset item comments through the CLI

linear[bot] commented 1 month ago

DAR-2294 Make the text argument for the "darwin dataset comment" command required

JBWilkie commented 1 month ago

To make an argument required, we can just pass required=True as one of it's argument

import argparse

if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("--text", type=str, help="Comment: list of words", required=True)
    args = parser.parse_args()

    print(args)
$ python t3.py
usage: t3.py [-h] --text TEXT
t3.py: error: the following arguments are required: --text

@saurbhc I thought about this approach, but in the example you shared if you run python t3.py --help, you get:

usage: test.py [-h] --text TEXT

optional arguments:
  -h, --help   show this help message and exit
  --text TEXT  Comment: list of words

Implying that text is optional, not required

saurbhc commented 1 month ago

@saurbhc I thought about this approach, but in the example you shared if you run python t3.py --help, you get:

usage: test.py [-h] --text TEXT

optional arguments:
  -h, --help   show this help message and exit
  --text TEXT  Comment: list of words

Implying that text is optional, not required

Hm, that's argparse's default logic of considering arguments starting with -- as 'optional arguments' see: https://stackoverflow.com/a/24181138

I suspect changing the --text -> text is a change in API format; maybe a breaking change for some users? 🤷

Idk if creating a required argument group would be over-engineering this? 🤔

import argparse

if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    required_grp = parser.add_argument_group("required arguments")
    required_grp.add_argument("--text", type=str, help="Comment: list of words", required=True)
    args = parser.parse_args()

    print(args)
$ python t3.py --help
usage: t3.py [-h] --text TEXT

options:
  -h, --help   show this help message and exit

required arguments:
  --text TEXT  Comment: list of words
JBWilkie commented 1 month ago

@saurbhc Damn that's annoying, this thread from 2010 explains why this wasn't changed

Having read that Stack Overflow thread, I think adding argument groups is probably needlessly complicated. I think I can live with -h & --help being slightly incorrect, since even if the required argument shows up under optional arguments, the lack of square brackets around the argument in usage indicates that it is in fact required

usage: test.py [-h] --text TEXT

Therefore have simply passed required=True for the argument