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

F401: Improve message for attempted re-export of symbols in __init__ files #5697

Closed
duckinator opened this issue Jul 11, 2023 · 7 comments · Fixed by #10365
Closed

F401: Improve message for attempted re-export of symbols in __init__ files #5697

duckinator opened this issue Jul 11, 2023 · 7 comments · Fixed by #10365
Labels
accepted Ready for implementation good first issue Good for newcomers

Comments

@duckinator
Copy link

Problem

A pattern I frequently use is to have __init__.py re-export __version__ from elsewhere, and run the release process whenever that file is modified on the main branch.

If I have __version__ = '1.2.3' directly in __init__.py, it's fine. If I do from .version import __version__, ruff complains about it being unused.

The real-world example where I encountered this is duckinator/bork.

Code

In blah/version.py:

__version__ = '1.2.3'

In blah/__init__.py:

from .version import __version__

Command & Output

The testcase consists of nothing but blah/version.py, blah/__init__.py.

puppy@orthrus.fox:~/ruff-testcase$ tree -a
.
└── blah
    ├── __init__.py
    └── version.py

2 directories, 2 files
puppy@orthrus.fox:~/ruff-testcase$ python3 -m venv ./venv && . venv/bin/activate && pip3 install ruff
Collecting ruff
  Using cached ruff-0.0.277-py3-none-freebsd_13_1_RELEASE_p6_amd64.whl
Installing collected packages: ruff
Successfully installed ruff-0.0.277

[notice] A new release of pip is available: 23.0.1 -> 23.1.2
[notice] To update, run: pip install --upgrade pip
(venv) puppy@orthrus.fox:~/ruff-testcase$ ruff --version
ruff 0.0.277
(venv) puppy@orthrus.fox:~/ruff-testcase$ cat blah/__init__.py
from .version import __version__
(venv) puppy@orthrus.fox:~/ruff-testcase$ cat blah/version.py
__version__ = '1.2.3'
(venv) puppy@orthrus.fox:~/ruff-testcase$ ruff check --isolated --select F401 blah/
blah/__init__.py:1:22: F401 [*] `.version.__version__` imported but unused
Found 1 error.
[*] 1 potentially fixable with the --fix option.
(venv) puppy@orthrus.fox:~/ruff-testcase$ 

Workaround

Just slap # noqa: F401 at the end of the line:

from .version import __version__  # noqa: F401

I feel like this shouldn't be necessary, though, which is why I opened this issue.

@duckinator
Copy link
Author

Also, __version__ becomes replaced with a bold-text "version" if you use --show-source lol

image

@zanieb
Copy link
Member

zanieb commented Jul 11, 2023

Hm interesting the bold text doesn't happen in general

example.py:1:1: F821 Undefined name `__test__`
  |
1 | __test__
  | ^^^^^^^^ F821
  |

Found 1 error.

but I can reproduce with your example.

example.py:1:22: F401 [*] `.version.__version__` imported but unused
  |
1 | from .version import __version__
  |                      ^^^^^^^^^^^ F401
  |
  = help: Remove unused import: `.version.version`

Found 1 error.
[*] 1 potentially fixable with the --fix option.

Regarding the original issue, you should "re-export" the name if you want it to be a part of the public interface of your module i.e.

from .version import __version__ as __version__

You can also define it in __all__

from .version import __version__

__all__ = ['__version__']

both of these will result in no violation being raised. See the Pyright library interface documentation for details on why that is.

@duckinator
Copy link
Author

Hm interesting the bold text doesn't happen in general [...] but I can reproduce with your example.

I wonder if the = help: lines may have more complex formatting enabled that isn't the case for some other things, perhaps?

Regarding the original issue, you should "re-export" the name if you want it to be a part of the public interface of your module i.e.

Thanks for the info. I didn't realize imports were considered private by default, somehow. Do you think it'd make sense -- possibly for __init__.py specifically -- to have a thing explaining as .../__all__? If not, I think this issue can probably be closed.

@zanieb
Copy link
Member

zanieb commented Jul 12, 2023

I wonder if the = help: lines may have more complex formatting enabled that isn't the case for some other things, perhaps?

I think you're right I reproduced this with another case. I've opened an issue for that at #5699

Do you think it'd make sense -- possibly for init.py specifically -- to have a thing explaining as .../all?

That does seem nice in __init__.py files especially! I updated the title, let me know if you think it makes sense!

@zanieb zanieb changed the title F401: Ruff complains about top-level re-export of __version__ F401: Improve message for attempted re-export of symbols in __init__ files Jul 12, 2023
@zanieb zanieb added help wanted Contributions especially welcome accepted Ready for implementation good first issue Good for newcomers and removed help wanted Contributions especially welcome labels Jul 12, 2023
@charliermarsh
Copy link
Member

We actually already support a custom message for these if you enable ignore-init-module-imports, although that setting is a bit strange... Perhaps it should be removed entirely, and we should just always show that custom message.

@charliermarsh
Copy link
Member

My vote would be to remove that setting and make it the default (and make the autofix "suggested" rather than "automatic" for __init__.py files, which IIRC is the reason the setting exists in the first place -- to avoid autofixing unused imports in those cases).

@edgarrmondragon
Copy link
Contributor

edgarrmondragon commented Jul 15, 2023

I'd like to work on this if it's up for grabs.

EDIT: #5845

zanieb added a commit that referenced this issue Mar 13, 2024
…to unsafe fix (#10365)

Re-implementation of #5845 but
instead of deprecating the option I toggle the default. Now users can
_opt-in_ via the setting which will give them an unsafe fix to remove
the import. Otherwise, we raise violations but do not offer a fix. The
setting is a bit of a misnomer in either case, maybe we'll want to
remove it still someday.

As discussed there, I think the safe fix should be to import it as an
alias. I'm not sure. We need support for offering multiple fixes for
ideal behavior though? I think we should remove the fix entirely and
consider it separately.

Closes #5697
Supersedes #5845

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation good first issue Good for newcomers
Projects
None yet
4 participants