astral-sh / ruff

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

`N999` (`invalid-module-name`) mixes correctness and stylistic recommendations #12475

Open edhinard opened 1 month ago

edhinard commented 1 month ago

Not only does rule N999 check that the PEP8 convention is respected, but also checks that the module name is syntactically correct. The latter is not part of PEP8. This is a problem for me currently working on "old non linted" code. I have to ignore this rule because of some modules whose names are syntactically correct but sometimes with capital letters. I think a E rule is missing that would only include the syntax checking of N999. (If I were you I would also remove that part from N999 for consistency)

MichaReiser commented 1 month ago

It doesn't seem that Python requires all module names to be valid identifiers. Only modules that are imported from other modules. But I might be wrong here. Ruff should always report a syntax error if you try to import a module with an invalid identifier name.

AlexWaygood commented 1 month ago

I believe @edhinard is complaining about something like the following directory structure:

They'd like a rule that complains about the naming of foo/not-an-identifier.py, as importing foo.not-an-identifier at runtime will fail. But they can't use our N999 rule (invalid-module-name) to flag this, because that would also flag foo/NotPep8Compliant.py). None of the other submodules in the foo package ever try to import not-an-identifier -- it's just exposed as a public submodule of the foo package -- so the bad name of the submodule also isn't caught by any of our rules that flag SyntaxErrors.

So I think the argument is that N999 should be split into two rules: one that flags invalid module names, and one that flags module names that are valid, but not PEP8-compliant. This seems like a reasonable argument to me, though it would mean we'd deviate from the behaviour of the original pep8-naming linter.

edhinard commented 1 month ago

You are right module names don't have to be valid identifiers:

$ touch 3.py
$ python3 -c "import importlib; print(importlib.import_module('3'))"
<module '3' from '/tmp/3.py'>

But it is not a good idea:

$ python3 -c "import 3"
  File "<string>", line 1
    import 3
           ^
SyntaxError: invalid syntax

and ruff is here to help us avoid that. IMHO the problem with N999 is that it combine two checks: 1) the pep8 convention which we cannot always adhere to, 2) the "dangerous permissiveness" of non regular module names. It is important to have 2) - for example sub-directories may be implicitely imported by pytest. Probably a new "E" rule is not a good idea. But something not mixed with pep8 convention

MichaReiser commented 1 month ago

@AlexWaygood would you support this check as a separate rule? @AlexElvers is interested in it.

AlexWaygood commented 1 month ago

@AlexWaygood would you support this check as a separate rule? @AlexElvers is interested in it.

Are you asking if I'd support splitting the existing check up (as the OP in this issue is requesting) or if I'd support adding an additional, more opinionated rule that also checked N999-type things for files that are not part of packages (as was proposed in https://github.com/astral-sh/ruff/pull/12535)?

For the first question: yes, definitely. For the second question: I think it could be a useful rule for some users, but I think it would probably be too opinionated for us to add now until we've done rule recategorisation.

MichaReiser commented 1 month ago

Oh, I thought it's both the same. Thanks for explaining and makes sense to me