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 #384

Merged
merged 12 commits into from
Jul 21, 2024
Merged

feat: add mypy plugin #384

merged 12 commits into from
Jul 21, 2024

Conversation

hiro-o918
Copy link
Contributor

@hiro-o918 hiro-o918 commented Jul 19, 2024

What

  • implement mypy plugin for gokart.TaskOnKart
    • This plugin checks the following when all the parameters are typed:
    • The parameters passed by the constructor are valid type
    • There are no missing parameters
image

@hiro-o918 hiro-o918 force-pushed the feature/mypy-plugin branch 2 times, most recently from eb899cb to 289286a Compare July 19, 2024 20:36
redis_port = luigi.OptionalParameter(default=None, description='Task lock check is deactivated, when None.', significant=False)
redis_timeout = luigi.IntParameter(default=180, description='Redis lock will be released after `redis_timeout` seconds', significant=False)
redis_host: Optional[str] = luigi.OptionalParameter(default=None, description='Task lock check is deactivated, when None.', significant=False)
redis_port: Optional[int] = luigi.OptionalIntParameter(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

redis_port should be int

@@ -81,7 +81,8 @@ def _test_run_with_empty_data_frame(cmdline_args: List[str], test_run_params: te

def try_to_run_test_for_empty_data_frame(cmdline_args: List[str]):
with CmdlineParser.global_instance(cmdline_args):
test_run_params = test_run()
# NOTE: parameters passed by command line cannot be linted by mypy.
test_run_params = test_run() # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mypy-plugin cannot pass this case even though the parameters passed from cmdlin_args

@hiro-o918 hiro-o918 force-pushed the feature/mypy-plugin branch 3 times, most recently from 2b8ca03 to 40eed14 Compare July 19, 2024 21:03
@@ -0,0 +1,369 @@
"""Plugin that provides support for gokart.TaskOnKart.

This Code reuses the code from mypy.plugins.dataclasses
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code quotes from mypy.plugins.dataclasses

if 'gokart.task.luigi.Task' in fullname:
# gather attibutes from gokart.TaskOnKart
# the transformation does not affect because the class has `__init__` method
return self._task_on_kart_class_maker_callback
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an entrypoint to transform a class to have a __init__ method with luigi. Parameters while type-checking for gokart.TaskOnKart class


sym = self.lookup_fully_qualified(fullname)
if sym and isinstance(sym.node, TypeInfo):
if any(base.fullname == 'gokart.task.TaskOnKart' for base in sym.node.mro):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also an entrypoint for inheritance of gokart.TaskOnKart

# then the object default __init__ with an empty signature will be present anyway.
if ('__init__' not in info.names or info.names['__init__'].plugin_generated) and attributes:
args = [attr.to_argument(info, of='__init__') for attr in attributes]
add_method_to_class(self._api, self._cls, '__init__', args=args, return_type=NoneType())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a point which create a method __init__ to a class while type checking

@hiro-o918 hiro-o918 marked this pull request as ready for review July 19, 2024 21:13
test_file.write(test_code.encode('utf-8'))
test_file.flush()
result = api.run(['--config-file', str(PYPROJECT_TOML), test_file.name])
self.assertIn('error: Missing named argument "bar" for "MyTask" [call-arg]', result[0])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are error messages for invalid arguments

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.

GREAT PR! Thanks!

@@ -64,6 +64,7 @@ quote-style = "single"

[tool.mypy]
ignore_missing_imports = true
plugins = ["gokart.mypy:plugin"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 👍 👍 👍 👍 👍

@yamamuteki
Copy link
Member

👍

Copy link
Member

@kitagry kitagry left a comment

Choose a reason for hiding this comment

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

Great PR 👍 I will review it thoroughly today.

gokart/mypy.py Outdated Show resolved Hide resolved
gokart/mypy.py Outdated Show resolved Hide resolved
docs/mypy_plugin.rst Outdated Show resolved Hide resolved
test/test_mypy.py Outdated Show resolved Hide resolved
gokart/mypy.py Outdated Show resolved Hide resolved
@hiro-o918 hiro-o918 force-pushed the feature/mypy-plugin branch 4 times, most recently from ded0be5 to ec0013c Compare July 21, 2024 07:25
Copy link
Member

@kitagry kitagry left a comment

Choose a reason for hiding this comment

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

LGTM!

@kitagry kitagry merged commit 8d5f4cc into master Jul 21, 2024
5 checks passed
@kitagry kitagry deleted the feature/mypy-plugin branch July 21, 2024 09:39
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.

4 participants