Skip to content

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

Closed
charliermarsh wants to merge 1 commit intomainfrom
charlie/f
Closed

Remove shadowed import, rather than shadowing import, in F811#10388
charliermarsh wants to merge 1 commit intomainfrom
charlie/f

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.

@github-actions
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.

1 participant