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

Enable Ruff flake8-import-conventions (ICN) #13731

Closed

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Mar 27, 2025

Ref #13295
flake8-import-conventions (ICN)

We were already using the import alias convention for numpy, tensorflow, matplotlib, pandas. Not for tkinter though (so @Akuli thoughts?)

@@ -204,6 +205,9 @@ ignore = [
[tool.ruff.lint.pydocstyle]
convention = "pep257" # https://docs.astral.sh/ruff/settings/#lint_pydocstyle_convention

[tool.lint.flake8-import-conventions.extend-aliases]
"numpy.typing" = "npt"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's a request to add this alias convention to the default aliases: astral-sh/ruff#17028

@Akuli
Copy link
Collaborator

Akuli commented Mar 27, 2025

I'm personally not a big fan of the import tkinter as tk convention, but it is quite popular, and I'm fine with merging this if other maintainers think this is a good idea.

That said, I don't see a strong reason to enforcing this rule in typeshed. For example, I don't see why it is a problem that networkx stubs do import numpy. Aliasing it to np makes sense if your project is full of numpy, but networkx stubs are not.

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Mar 27, 2025

stdlib/tkinter/ttk.pyi's Style.tk was also causing issues for mypy:

class Style:
    master: Incomplete
    tk: _tkinter.TkappType
    def __init__(self, master: tk.Misc | None = None) -> None: ...

https://github.com/python/typeshed/actions/runs/14118275408/job/39553451357?pr=13731#step:6:17

Hard to justify using a different alias in that one place when it's all within tkinter's own stub anyway.

This comment has been minimized.

pyproject.toml Outdated
"pandas"="pd"
"seaborn"="sns"
"tensorflow"="tf"
# "tkinter"="tk" # Causes name conflict in stdlib/tkinter/ttk.py with Style.tk
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if you could still use extend-aliases and set "tkinter"="tkinter" to disable that one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That actually works! Even if we don't end up using these rules, I opened a request to document that behaviour: astral-sh/ruff#17097

@JelleZijlstra
Copy link
Member

I don't think this is worth it; there's no real point for us in using these rules.

@AlexWaygood
Copy link
Member

do these get autofixed? If so then I don't mind them -- there is value in consistency, even if it's not very much value.

If they don't get autofixed then I definitely agree then they're just too annoying to be worth it

@Avasam
Copy link
Collaborator Author

Avasam commented Mar 31, 2025

do these get autofixed?

The changes here are all autofixes. But they're "unsafe" autofixes. So you have to explicitly run ruff check --fix --unsafe-fixes or use editor hint actions. It won't be fixed by editors on save, pre-commit hooks, CI, etc. unless we explicitly allow this one with extend-safe-fixes (like we do UP036)

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood
Copy link
Member

does the rule document why the fixes are unsafe?

@JelleZijlstra
Copy link
Member

I feel even if it's autofixable, the fact that we need to add a noqa rule in one case means the costs are higher than the benefits.

@AlexWaygood
Copy link
Member

fair enough

@Avasam
Copy link
Collaborator Author

Avasam commented Mar 31, 2025

does the rule document why the fixes are unsafe?

It doesn't, but i think it's clear that it changes that available and used symbol names, which could result in a clash or unexpected dynamic result (like from a getattr or and analysis tool). Feel free to add it to the doc if you want.

@Avasam
Copy link
Collaborator Author

Avasam commented Mar 31, 2025

Looks like there's a consensus on "not worth automating".

@Avasam Avasam closed this Mar 31, 2025
@Avasam Avasam deleted the Enable-Ruff-flake8-import-conventions-(ICN) branch March 31, 2025 17:58
@AlexWaygood
Copy link
Member

In general Ruff's aim to explicitly document why any given fix is considered unsafe. We obviously fall a long way short of that aim right now! It looks like it's one of the rules listed in astral-sh/ruff#15584

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.

4 participants