Skip to content

Comments

enforce required imports even with useless alias#14287

Merged
dylwil3 merged 10 commits intoastral-sh:mainfrom
dylwil3:isort-alias
Nov 14, 2024
Merged

enforce required imports even with useless alias#14287
dylwil3 merged 10 commits intoastral-sh:mainfrom
dylwil3:isort-alias

Conversation

@dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Nov 11, 2024 •
edited
Loading

This PR handles a panic that occurs when applying unsafe fixes if a user inserts a required import (I002) that has a "useless alias" in it, like import numpy as numpy, and also selects PLC0414 (useless-import-alias)

In this case, the fixes alternate between adding the required import statement, then removing the alias, until the recursion limit is reached. See linked issue for an example.

Tests to confirm panic resolved:

$ echo 1 | cargo run --quiet -p ruff -- check --no-cache --isolated --select I002,PLC0414 --config 'lint.isort.required-imports = ["from module import this as this"]' - --unsafe-fixes --fix
from module import this as this
1
Found 1 error (1 fixed, 0 remaining).
$ echo 1 | cargo run --quiet -p ruff -- check --no-cache --isolated --select I002,PLC0414 --config 'lint.isort.required-imports = ["import numpy as numpy"]' - --unsafe-fixes --fix
import numpy as numpy
1
Found 1 error (1 fixed, 0 remaining).

Closes #14283

@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 11, 2024

Not so happy about the big diff in such a simple rule for an edge case... I welcome alternative suggestions!

@github-actions
Copy link
Contributor

github-actions bot commented Nov 11, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser
MichaReiser reviewed Nov 12, 2024
View reviewed changes
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.

Could you expand the PR summary with details about what was causing the infinite loop or panic? It would help me review this PR and possibly suggest alternatives.

@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 12, 2024 •
edited
Loading

Done! It also occurred to me that it might be better to keep the diagnostic but elide the fix for useless-import-alias. That would serve the purpose of warning the user that something was amiss, and they could override the warning by ignoring the rule.

Edit: Made this change. I did not change the "Fix availability" or documentation because it seems like this is a very rare (nonexistent?) exception, but let me know what you think.

@charliermarsh
Copy link
Member

charliermarsh commented Nov 13, 2024

Do you mind adding a dedicated test for this? Sorry!

@MichaReiser MichaReiser added bug Something isn't working fixes Related to suggested fixes for violations labels Nov 13, 2024
@dylwil3
Copy link
Collaborator Author

dylwil3 commented Nov 14, 2024

Do you mind adding a dedicated test for this? Sorry!

Added!

MichaReiser
MichaReiser approved these changes Nov 14, 2024
View reviewed changes
@dylwil3 dylwil3 merged commit 8095ff0 into astral-sh:main Nov 14, 2024
@dylwil3 dylwil3 deleted the isort-alias branch November 14, 2024 21:39
@dylwil3 dylwil3 mentioned this pull request Nov 14, 2024
Merged
dylwil3 added a commit that referenced this pull request Nov 14, 2024
@dylwil3
[pylint] Remove check for dot in alias name in `useless-import-alia…
ba6c7f6 
…s (PLC0414)` (#14345)

Follow-up to #14287 : when checking that `name` is the same as `as_name`
in `import name as as_name`, we do not need to first do an early return
if `'.'` is found in `name`.
@BrewTestBot BrewTestBot mentioned this pull request Nov 15, 2024
Merged
@dylwil3 dylwil3 restored the isort-alias branch November 17, 2024 14:28
@yunchipang yunchipang mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@charliermarsh charliermarsh charliermarsh left review comments

Assignees

No one assigned

Labels

bug Something isn't working fixes Related to suggested fixes for violations

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Infinite loop] PLC0414 conflicts with I002 if a useless alias is required

3 participants

@dylwil3 @charliermarsh @MichaReiser