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

Remove shadowed import, rather than shadowing import, in F811 #10388

Closed
wants to merge 1 commit into from

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 13, 2024

Summary

Today, in F811, if you have code like:

import datetime
from datetime import datetime

We flag a violation on the second line (the line of the redefinition), and the fix we generate is a removal of the second import. In #10387, I removed the fix in these cases, when the imports map to different symbols.

It seems "more correct", though to remove the first import, since the second import is the one that will actually be used in practice. This PR changes that behavior.

However, I'm sure if this is actually correct, because we're now changing code that's far away from the violation itself. There's an example in our test suite that shows why this isn't ideal:

from typing import (
    Sequence  # noqa
)

from typing import (
    Sequence
)

In this case, we'd remove the Sequence # noqa line, since the # noqa isn't on the line containing the F811 violation. (I'm not really interested in parsing out the # noqa violation from the Sequence # noqa line, because it risks breaking a bunch of assumptions about how # noqa works.)

Anyway, interested in feedback.

@charliermarsh
Copy link
Member Author

I think this isn't a great idea.

Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant