-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Verify decorators parameter in __init__.pyi matches operator #51048
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
base: main
Are you sure you want to change the base?
Conversation
I like the idea, but this feels like it belongs to a pre-commit check rather than a unit test. |
yes that makes sense. let me add this as part of precommit test. |
87a9768
to
444ba87
Compare
@uranusjr There are some complexities in moving to pre-commit. As unit tests, they run within the virtualenv and are context-aware, running without issue. But to run as part of a pre-commit hook, setting up the path has to be done. even when done,
|
@uranusjr when you have time, could you help me handle the above issue? |
@amoghrajesh @potiuk @uranusjr If any of you get time, could you help me in understanding if this can be better handled? |
@Bowrna I also think its not a great idea to add this in unit tests but in precommit checks to catch things earlier and repeatedly. Did you try using the inspect library and matching signatures.
Tried with the console and it works pretty consistently. |
5945451
to
19f3114
Compare
c3ec851
to
a2a98fd
Compare
6d67af2
to
28243ff
Compare
278eac1
to
bc25384
Compare
@uranusjr could you review this PR and share your feedback? |
20aaca2
to
d7ffbc6
Compare
@uranusjr the fix you suggested is added into this. thank you. when you get, please verify this PR and share your feedback. |
@uranusjr do i have any update regarding this PR? |
957ae00
to
5ea92d3
Compare
"short_circuit": "airflow.providers.standard.operators.python.ShortCircuitOperator", | ||
"python": "airflow.providers.standard.operators.python.PythonOperator", | ||
"external_python": "airflow.providers.standard.operators.python.ExternalPythonOperator", | ||
# Add more here... |
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.
Hmm. I think it would be great to have some way to detect when we need to add something here - likely importing everything and looking for anythiing deriving from decorator? We do similar things "verify providers" pre-comit check when we walk all classes and find ones that derive from BaseHook for example.
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 wonder how we can prevent people from forgetting. Maybe a separate pre-commit hook that checks if a commit adds an entry to the provider config (
task-decorators
inprovider.yaml
) but does not also change this? Or maybe we should just pull the list of classes fromprovider.yaml
directly instead? This can probably be explored in a second PR after this is merged.
@uranusjr suggested this one @potiuk . could i add that as seperate PR after this one gets merged?
related: #48448
closes: #48448
Add tests to all existed decorators to make sure the parameter list they accept matches the operator.
In this PR, I have added tests to verify the match using the libcst library. i have included tests for few decorators and once i get reviews if this way of verifying is fine, i will add the other decorator case too. cc: @eladkal
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in airflow-core/newsfragments.