astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.23k stars 1.08k forks source link

feature request: report module missing `__all__` #2098

Open sbrugman opened 1 year ago

sbrugman commented 1 year ago

Background

For python packages it would be helpful to notify when a non-empty module lacks __all__. Having __all__ present forces module developers to continuously decide on sub-modules that should be imported. In practice is prevents errors such as unused imports.

This is typically one of the checks to have when a code base is maturing, and fits with other best practices. At first glance, this is what flake8-dunder-all provides. The actual implementation however, is close to pyflakes F401: unused-import. Hence, DAL001 can be considered an alias for F401.

Proposal

My proposal is to implement the functionality under the code DAL002. ruff already has quite some functionality related to imports, and this feature should require little code. Autofix is possible and there have been tools that are doing this.

Examples

ruff currently reposes on __init__.py files:

Example __init__.py:

from sub.module import foo, bar

foo(3)

F401 sub.module.bar imported but unused (configured by ignore-init-module-imports)

But even for an __init__.py file without unused imports ruff would be welcome to suggest __all__, and ideally offering the possibility to autofix in most cases:

from sub.module import foo, bar

foo(3)
bar(2)

Preferably should hint that:

DAL002: module lacks __all__

charliermarsh commented 1 year ago

Gives no error in my setup (despite ignore-init-module-imports should be false).

Interesting... Separate from this proposal, that sounds like a bug, but I can't reproduce:

❯ cat foo/__init__.py
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: foo/__init__.py
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ from sub.module import foo, bar
   2   │
   3   │ foo(3)
❯ cargo run foo -n --isolated
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/ruff foo -n --isolated`
foo/__init__.py:1:29: F401 `sub.module.bar` imported but unused
Found 1 error(s).
1 potentially fixable with the --fix option.
charliermarsh commented 1 year ago

Hence, DAL001 can be considered a synonym of F401.

I don't think I fully follow this part... The documentation of the plugin suggests that it does exactly what you describe 😅 Does it do something different in practice?

charliermarsh commented 1 year ago

Are there any files that, conventionally, should be able to omit an __all__ export? Like "private" modules?

bluetech commented 1 year ago

Are there any files that, conventionally, should be able to omit an all export? Like "private" modules?

In the typing world there is a convention that from .foo import bar as bar is considered an export. This is the rule implemented by mypy's implicit_reexport check, for example (it also supports __all__), but it is not understood at runtime (doesn't affect star imports like __all__ does). However I don't think it should affect the lint; if one is using this convention, can just not use the lint.

sbrugman commented 1 year ago

I don't think I fully follow this part... The documentation of the plugin suggests that it does exactly what you describe 😅 Does it do something different in practice?

The wording of the violation "module lacks __all__" is unfortunate. It chooses to add __all__ when there are unused imports. It also scans on file level, not on module level/___init__.py as the message suggests (see tests for examples).

sbrugman commented 1 year ago

Interesting... Separate from this proposal, that sounds like a bug, but I can't reproduce:

I'll give it another try. Could be that my test just missed isolation. If it persists, I'll open another issue.

silasary commented 1 year ago

It also scans on file level, not on module level/_init.py as the message suggests

As someone who uses flake8-dunder-all, I want it to scan all modules, not just __init__.py files (packages).
The biggest value of __all__ is in files with logic, because you don't want to be re-exporting your dependencies. It's slightly useful in __init__.py files, but only because it does count as a use in the context of F401.

CarrotManMatt commented 9 months ago

I would also really appreciate this rule being implemented, especially if it scans every module (not just __init__.py files)