-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: add --bulk option to add task command #11
Conversation
"--details", | ||
"details", | ||
cls=NotRequired, | ||
not_required=["bulk"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be not_required_if
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's two different classes, see here: parameters.py
The first one NotRequiredIf
is a click.Argument
class, it allows marking a command arg as "not required if" the following parameters are present. The second one NotRequired
is a click.Option
class, it raises an error if one of the given parameters is present with the option.
I guess it's time for me to add some docstrings!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood you well, it means that the title
argument is not required when the bulk
option is setup. But what if both title
and bulk
are presents ? Shouldn't it raise an error ?
I mean, the bulk
option seems to be incompatible with both details
and title
since those two last arguments refer to a single task, while bulk
introduces several tasks.
If indeed it is incompatible, either one should raise an error when setup along with bulk
, what do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fine.
#9