-
Notifications
You must be signed in to change notification settings - Fork 133
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
Raise error on empty arguments to group
#1008
Conversation
Great, thanks! If you rebase off of Otherwise mind writing a test? Should be as simple as adding a line here with an empty args/kwargs, probably don't have to run locally.
|
@@ -176,6 +176,9 @@ def _validate_group_params( | |||
f"Dependency: {dependency} is not a valid dependency type for group(), must be " | |||
f"a LiteralDependency or UpstreamDependency." | |||
) | |||
raise InvalidDecoratorException( |
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 will always raise, no? Will want to add return
statements above or something to get it to work.
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.
Or an if
statement for both of them
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.
Oops! Thanks 👍
Looking good! Mind rebasing off of |
add test for invalid dependencies
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.
Looks good, thank you!
@@ -176,6 +176,9 @@ def _validate_group_params( | |||
f"Dependency: {dependency} is not a valid dependency type for group(), must be " | |||
f"a LiteralDependency or UpstreamDependency." | |||
) | |||
raise InvalidDecoratorException( |
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.
Or an if
statement for both of them
Fixes #1007
How I tested this
Hand testing.
Checklist