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

Allow sys.path modifications between imports #9047

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Dec 7, 2023

Summary

It's common to interleave a sys.path modification between imports at the top of a file. This is a frequent cause of # noqa: E402 false positives, as seen in the ecosystem checks. This PR modifies E402 to omit such modifications when determining the "import boundary".

(We could consider linting against sys.path modifications, but that should be a separate rule.)

Closes: #5557.

Copy link
Contributor

github-actions bot commented Dec 7, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+23 -2 violations, +0 -0 fixes in 41 projects)

RasaHQ/rasa (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ .github/tests/test_download_pretrained.py:10:29: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ .github/tests/test_mr_generate_summary.py:4:49: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ .github/tests/test_mr_publish_results.py:7:35: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ .github/tests/test_validate_gpus.py:8:22: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ .github/tests/test_validate_gpus.py:9:23: RUF100 [*] Unused `noqa` directive (unused: `E402`)

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

ruff check --no-cache --exit-zero --preview --select ALL

+ docs/conf.py:50:69: RUF100 [*] Unused `noqa` directive (unused: `E402`)

freedomofpress/securedrop (+15 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ securedrop/manage.py:22:16: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/manage.py:23:47: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/manage.py:29:20: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/manage.py:30:55: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/manage.py:31:33: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/manage.py:32:56: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/manage.py:33:39: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/manage.py:42:80: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/scripts/rqrequeue:11:40: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/scripts/rqrequeue:12:46: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/scripts/shredder:14:24: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/scripts/shredder:15:40: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/scripts/shredder:16:28: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/scripts/source_deleter:14:24: RUF100 [*] Unused `noqa` directive (unused: `E402`)
+ securedrop/scripts/source_deleter:15:40: RUF100 [*] Unused `noqa` directive (unused: `E402`)

model-bakers/model_bakery (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ docs/conf.py:6:37: RUF100 [*] Unused blanket `noqa` directive

pypa/pip (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview

+ noxfile.py:16:42: RUF100 [*] Unused blanket `noqa` directive

zulip/zulip (+0 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

- docs/conf.py:11:1: E402 Module level import not at top of file
- zerver/lib/test_fixtures.py:21:1: E402 Module level import not at top of file

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
RUF100 23 23 0 0 0
E402 2 0 2 0 0

@charliermarsh
Copy link
Member Author

I think these are all improvements -- it's nice to see so many # noqa comments being removed.

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Dec 7, 2023
@charliermarsh charliermarsh changed the title Allow sys.path modifications between imports Allow sys.path modifications between imports Dec 7, 2023
@charliermarsh charliermarsh marked this pull request as ready for review December 7, 2023 17:09
@charliermarsh
Copy link
Member Author

charliermarsh commented Dec 7, 2023

I gated this to preview for now (ecosystem checks are still updating at time of writing), though honestly I'd be okay with shipping it to stable...

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Nice! I think preview makes sense for this change.

@charliermarsh charliermarsh merged commit b021ede into main Dec 7, 2023
17 checks passed
@charliermarsh charliermarsh deleted the charlie/E402 branch December 7, 2023 18:35
@konstin
Copy link
Member

konstin commented Dec 7, 2023

Looking at the ecosystem checks with entrypoint scripts and tests, this makes even more sense

@tooruu
Copy link

tooruu commented Dec 14, 2023

Why modify sys.path? This sounds like a dirty fix for bad design, not something to encourage.

@zanieb
Copy link
Member

zanieb commented Dec 14, 2023

Hi @tooruu I'd recommend looking through the ecosystem checks as some of this usage is entirely valid.

We're not encouraging this behavior, we're simply fixing a false positive for a different lint rule. We can introduce a separate rule to suggest not making sys.path modifications to properly discourage this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

module-import-not-at-top-of-file ignore after sys.path.insert
4 participants