-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: add parameter.pyi for type hinting parameters #3370
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
c21d4e7 to
ace37cc
Compare
dlstadther
left a comment
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.
1 question, 1 request
ace37cc to
09304cf
Compare
|
@RRap0so The codecov check is blocking me from merging this PR. Would you be able to help here? |
09304cf to
68d40bf
Compare
|
Hello. Is there any updates? |
|
I don't have permissions to override or disable the one failing coverage test. That test has always been temperamental. It used to not be required to merge. But since it became required, a Spotify employee, such as @RRap0so , must assist. |
|
@kitagry The codecov/changes check is failing because luigi/mypy.py has an indirect coverage drop (-0.74%). The uncovered lines are in the except ValueError handler: Could you add a test case in test/mypy_test.py that exercises this path? You'll need a Parameter subclass where "default" is not in ctx.callee_arg_names — for example, a Parameter instantiated without a default argument. @dlstadther looks like something changed and I can't just easily bypass things. I'll have to take a look but meanwhile that's a way. We can also disable this check if we feel like it doesn't bring much value. Let me know if you would prefer that and ping me on the PR so I can approve. |
dc88071 to
97235bd
Compare
97235bd to
044a2cd
Compare
044a2cd to
d42cae4
Compare
|
Thank you! I'm looking forward to the next release😍 |
Description
This PR adds a type stub file (
parameter.pyi) for Luigi's Parameter classes to provide better type hints and IDE support. Additionally, it applies the@dataclass_transformdecorator to theTaskclass to enable dataclass-like type checking behavior.Key changes:
luigi/parameter.pyistub file with type definitions for all Parameter classes@dataclass_transformdecorator toTaskclass for better type checker supporttyping-extensions>=4.12.2as a dependencyGenerictypes,TypedDict, andUnpackfor precise type annotationsMotivation and Context
This change improves the developer experience when using Luigi by providing:
@dataclass_transformThe stub file approach allows us to provide rich type information without impacting runtime performance or modifying the existing implementation. The
@dataclass_transformdecorator helps type checkers understand that Task classes behave similarly to dataclasses when it comes to parameter handling.This enhancement helps catch type-related bugs earlier in the development process and makes the codebase more maintainable.
Implementation Details
.pyifile provides type signatures for all Parameter variants including specialized types likeEnumParameter,ChoiceParameter,NumericalParameter, etc.EnumParameter[EnumParameterType],ChoiceParameter[ChoiceType])TypedDictwithUnpackprovides typed keyword arguments for parameter constructors@dataclass_transformdecorator withfield_specifiers=(Parameter,)enables proper type checking for Task classesHave you tested this? If so, how?
Yes, I have tested this implementation with multiple type checkers:
✅ Working Type Checkers
.pyistub file.pyistub file@dataclass_transformwithfield_specifiers. However, mypy supports custom plugins, so users can use Implement mypy plugin forluigi.Task#3315@dataclass_transform. https://github.com/astral-sh/ruff/blob/dea48ecef06803502a265488e4a16b3e35778a57/crates/ty_python_semantic/src/types.rs#L7153-L7155Test Example