Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

JBWilkie
Copy link
Collaborator

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

Copy link

linear bot commented May 24, 2024

Copy link
Member

@saurbhc saurbhc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
$ python t3.py --text 1
Namespace(text='1')

@JBWilkie
Copy link
Collaborator Author

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
Copy link
Member

saurbhc commented May 24, 2024

@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
Copy link
Collaborator Author

JBWilkie commented May 29, 2024

@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

@JBWilkie JBWilkie merged commit b0609b9 into master Jun 3, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants