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

feat: add mypy plugin options to handle missing paramters for a task #428

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hiro-o918
Copy link
Contributor

@hiro-o918 hiro-o918 commented Jan 31, 2025

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.

Copy link
Collaborator

@hirosassa hirosassa left a 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
Copy link
Member

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?

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

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.

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.

3 participants