-
Notifications
You must be signed in to change notification settings - Fork 62
feat: add mypy plugin options to handle missing paramters for a task #428
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
Conversation
6042033
to
b8d0720
Compare
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.
LGTM!
module = ["pandas.*", "apscheduler.*", "dill.*", "boto3.*", "testfixtures.*", "luigi.*"] | ||
|
||
[tool.gokart-mypy] | ||
error_on_missing_parameters = true |
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.
I think this name is a little long. And the name of mypy is something like disallow-
or ignore-
. Therefore, I think disallow_missing_parameters
is preferable. 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.
updated the param name
gokart/mypy.py
Outdated
config = cls._parse_toml(config_file) | ||
gokart_plugin_config = config.get('tool', {}).get('gokart-mypy', {}) | ||
|
||
error_on_missing_parameters = gokart_plugin_config.get('error_on_missing_parameters', False) |
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.
I think parameter name should be FINAL variable.
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.
I introduce Enum for parameters
857b40d
to
f8432e7
Compare
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.
@hiro-o918 Sorry for the late review. Could you add documentation?
9af5f47
to
ddf8e6a
Compare
Sorry for late response, I added docs |
ddf8e6a
to
d89dcb3
Compare
Thank you!! |
Background
In this discussion, I initially omitted handling errors for missing parameters in the task constructor because luigi.Config allows implicit parameters, making it difficult to validate them on linting
However, I occasionally forget to pass required parameters to tasks, leading to runtime errors when executing the pipeline.
What
This PR introduces an option for gokart mypy to raise an error when a task constructor is called without all required parameters.
Users can configure mypy to allow implicit parameter passing if needed in their project.