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

Enable TCH004 and TCH005 rules #35475

Merged
merged 1 commit into from Nov 7, 2023
Merged

Conversation

Taragolis
Copy link
Contributor

Follow-up: #35465 (comment)

Enable two additional rules TCH rules in ruff

  1. TCH004 - Check that import only use as type annotation, has false positive alarms so I think that is why it under the --unsafe-fixes
airflow/dag_processing/processor.py:66:36: TCH004 Move import `airflow.models.dag.DAG` out of type-checking block. Import is used for more than type hinting.
airflow/serialization/pydantic/job.py:27:34: TCH004 Move import `airflow.jobs.job.Job` out of type-checking block. Import is used for more than type hinting.
  1. TCH005 - Remove empty TYPE_CHECKING block

^ 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 newsfragments.

@Taragolis
Copy link
Contributor Author

Move to draft for prevent accidentally merge into the main

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice.

@Taragolis
Copy link
Contributor Author

Need to check that nothing actually broken after this changes.

But seems like it should fix some methods in:

  • airflow/dag_processing/processor.py
  • airflow/serialization/pydantic/job.py

@Taragolis
Copy link
Contributor Author

Yeah.. Circular imports

ImportError: cannot import name 'Job' from partially initialized module 'airflow.jobs.job' (most likely due to a circular import) (/opt/airflow/airflow/jobs/job.py)

Well local imports then

Comment on lines -83 to +85
from kubernetes.client import models as k8s
from kubernetes.client import models as k8s # noqa: TCH004

from airflow.providers.cncf.kubernetes.pod_generator import PodGenerator
from airflow.providers.cncf.kubernetes.pod_generator import PodGenerator # noqa: TCH004
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The real dark magic happen here

elif var.__class__.__name__ == "V1Pod" and _has_kubernetes() and isinstance(var, k8s.V1Pod):
json_pod = PodGenerator.serialize_pod(var)
return cls._encode(json_pod, type_=DAT.POD)

That is only affect k8s and PodGenerator so better exclude only this lines from check rather then entire module

@Taragolis Taragolis removed the provider:microsoft-azure Azure-related issues label Nov 6, 2023
@Taragolis Taragolis marked this pull request as ready for review November 6, 2023 12:07
@uranusjr uranusjr merged commit b59b205 into apache:main Nov 7, 2023
46 checks passed
@Taragolis Taragolis deleted the another-tch-rules branch November 7, 2023 06:05
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Nov 10, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Nov 20, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler Scheduler or dag parsing Issues area:serialization type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants