Skip to content

Allow sys.path modifications between imports#9047

Merged
charliermarsh merged 1 commit intomainfrom
charlie/E402
Dec 7, 2023
Merged

Allow sys.path modifications between imports#9047
charliermarsh merged 1 commit intomainfrom
charlie/E402

Conversation

@charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Dec 7, 2023 •
edited
Loading

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.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2023 •
edited
Loading

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

charliermarsh commented Dec 7, 2023

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 •
edited
Loading

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

zanieb
zanieb approved these changes Dec 7, 2023
View reviewed changes
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
@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

@krehel krehel mentioned this pull request Dec 13, 2023
Merged
@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.

@Pastells Pastells mentioned this pull request Feb 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zanieb zanieb zanieb approved these changes

@konstin konstin Awaiting requested review from konstin

Assignees

No one assigned

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@charliermarsh @konstin @tooruu @zanieb