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

Honor banned top level imports by TID253 in PLC0415. #15628

Merged

Conversation

mishamsk
Copy link
Contributor

Summary

Fixes #12803

I didn't dare to extract the shared module matching logic from tidy-imports into some helper module, so there is some questionable cross-use between lints.

Test Plan

Added new snapshot test

Copy link
Contributor

github-actions bot commented Jan 21, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -66 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)

apache/airflow (+0 -30 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

- airflow/example_dags/example_branch_operator.py:134:9: PLC0415 `import` should be at the top-level of a file
- airflow/example_dags/example_branch_operator.py:155:9: PLC0415 `import` should be at the top-level of a file
- airflow/example_dags/example_branch_operator_decorator.py:119:9: PLC0415 `import` should be at the top-level of a file
- airflow/example_dags/example_branch_operator_decorator.py:138:13: PLC0415 `import` should be at the top-level of a file
- airflow/example_dags/tutorial_objectstorage.py:73:9: PLC0415 `import` should be at the top-level of a file
- airflow/serialization/serializers/numpy.py:50:5: PLC0415 `import` should be at the top-level of a file
- airflow/serialization/serializers/pandas.py:39:5: PLC0415 `import` should be at the top-level of a file
- providers/celery/src/airflow/providers/celery/executors/celery_executor_utils.py:122:9: PLC0415 `import` should be at the top-level of a file
- providers/edge/src/airflow/providers/edge/example_dags/integration_test.py:95:9: PLC0415 `import` should be at the top-level of a file
- providers/edge/src/airflow/providers/edge/example_dags/win_test.py:276:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/amazon/aws/hooks/glue.py:478:9: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/amazon/aws/transfers/sql_to_s3.py:158:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/amazon/aws/transfers/sql_to_s3.py:159:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/amazon/aws/transfers/sql_to_s3.py:211:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/apache/hive/hooks/hive.py:1054:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/common/sql/hooks/sql.py:368:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/common/sql/hooks/sql.py:395:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/oracle/hooks/oracle.py:277:13: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/presto/hooks/presto.py:170:9: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/salesforce/hooks/salesforce.py:249:9: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/salesforce/hooks/salesforce.py:250:9: PLC0415 `import` should be at the top-level of a file
- providers/src/airflow/providers/salesforce/hooks/salesforce.py:367:9: PLC0415 `import` should be at the top-level of a file
... 8 additional changes omitted for project

bokeh/bokeh (+0 -36 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

- src/bokeh/core/property/bases.py:256:13: PLC0415 `import` should be at the top-level of a file
- src/bokeh/core/property/pd.py:59:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/core/property/pd.py:78:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/core/property/visual.py:154:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/core/property/visual.py:168:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/core/serialization.py:459:13: PLC0415 `import` should be at the top-level of a file
- src/bokeh/document/events.py:518:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/export.py:281:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/export.py:368:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/export.py:369:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/webdriver.py:71:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/webdriver.py:72:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/webdriver.py:73:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/webdriver.py:74:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/webdriver.py:90:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/webdriver.py:91:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/io/webdriver.py:92:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/models/sources.py:227:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/models/sources.py:257:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/models/sources.py:373:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/models/sources.py:508:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/models/util/structure.py:308:9: PLC0415 `import` should be at the top-level of a file
- src/bokeh/plotting/_plot.py:76:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/plotting/_plot.py:77:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/sampledata/airports.py:68:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/sampledata/anscombe.py:82:5: PLC0415 `import` should be at the top-level of a file
- src/bokeh/sampledata/antibiotics.py:89:5: PLC0415 `import` should be at the top-level of a file
... 9 additional changes omitted for project

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PLC0415 66 0 66 0 0

@MichaReiser
Copy link
Member

The ecosystem changes look off. Would you mind taking a look? E.g. I don't think random is a banned import but for some reason, it no longer gets flagged in https://github.com/apache/airflow/blob/a76af4f22c0f34813ec51f00cd0e2a4909c77cbf/airflow/example_dags/example_branch_operator.py#L132

@mishamsk
Copy link
Contributor Author

The ecosystem changes look off. Would you mind taking a look? E.g. I don't think random is a banned import but for some reason, it no longer gets flagged in https://github.com/apache/airflow/blob/a76af4f22c0f34813ec51f00cd0e2a4909c77cbf/airflow/example_dags/example_branch_operator.py#L132

yes, sure, I will. I wanted to run these checks locally yesterday, but it was getting late, so I haven’t had time to figure that out. First time making smth. with a lint. Thanks for the heads up

extend test cases to cover all variations
@mishamsk
Copy link
Contributor Author

@MichaReiser I believe this is now all fixed. Spot checked half of Airflow changes, all numpy/pandas that are indeed banned by TID253 here

and checked one case in bokeh, PIL is also banned here

Let me know if this looks good!

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks. This looks good from a functionality but we should avoid collecting the names when calling the checker to avoid unnecessary overhead. I suggest moving the logic into import_outside_top_level and extract the names from stmt.

crates/ruff_linter/src/checkers/ast/analyze/statement.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser merged commit 17a8a55 into astral-sh:main Jan 24, 2025
21 checks passed
@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jan 24, 2025
@mishamsk mishamsk deleted the 12803-fix-tid253-incompatible-with-plc0415 branch January 24, 2025 12:38
dcreager added a commit that referenced this pull request Jan 24, 2025
* main:
  Add `check` command (#15692)
  [red-knot] Use itertools to clean up `SymbolState::merge` (#15702)
  [red-knot] Add `--ignore`, `--warn`, and `--error` CLI arguments (#15689)
  Use `uv init --lib` in tutorial (#15718)
  [red-knot] Use `Unknown | T_inferred` for undeclared public symbols (#15674)
  [`ruff`] Parenthesize fix when argument spans multiple lines for `unnecessary-round` (`RUF057`) (#15703)
  [red-knot] Rename `TestDbBuilder::typeshed` to `.custom_typeshed` (#15712)
  Honor banned top level imports by TID253 in PLC0415.  (#15628)
  Apply `AIR302`-context check only in `@task` function (#15711)
  [`airflow`] Update `AIR302` to check for deprecated context keys (#15144)
  Remove test rules from JSON schema (#15627)
  Add two missing commits to changelog (#15701)
  Fix grep for version number in docker build (#15699)
  Bump version to 0.9.3 (#15698)
  Preserve raw string prefix and escapes (#15694)
  [`flake8-pytest-style`] Rewrite references to `.exception` (`PT027`) (#15680)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import-outside-top-level (PLC0415) is incompatible with flake8-tidy-imports.banned-module-level-imports
2 participants