Update disallowed-name to flag module-level variables

[clavedeluna]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #3701

The original issue brought up the fact that foo = {}.keys() was not getting flagged for disallowed-name. I discovered that in some past refactoring, one else: case wasn't included, which is checking all module-level var names.

[clavedeluna]

I'm going to also point out a couple interesting examples from functional tests that don't seem to be flagged correctly.

In this first example, why would only Alias5 and Alias6 be flagged, and not all?

import collections.abc
from collections.abc import Callable
from typing import TYPE_CHECKING, Optional, Union

Alias1 = Optional[Callable[[int], None]]  # [broken-collections-callable]
Alias2 = Union[Callable[[int], None], None]  # [broken-collections-callable]
Alias3 = Optional[Callable[..., None]]
Alias4 = Union[Callable[..., None], None]
Alias5 = list[Callable[..., None]] # this is getting flagged as invalid-name
Alias6 = Callable[[int], None] # this is getting flagged as invalid-name

in this example, the first is a constant, so it's good, but the second SOME_CLASS_VAR isn't a constant. Shouldn't this be flagged as invalid-name, and an author should instead use lower_case?

class Something:
    SOME_CLASS_CONST = 1
    SOME_CLASS_VAR = [1, 2]  # shouldn't this be  [invalid-name]?

[DanielNoord]

I don't really understand what you're changing in this PR, but this change is incorrect.

DUMMY is a "constant" in pylint/Python terms and should be uppercased.

[clavedeluna]

So full disclosure I was very hesitant with these code changes (the first commit is the only important one) bc I was surprised pylint wasn't doing this initially, but then I posted test cases in the issue and was confirmed we needed this change #3701 (comment)

My interpretation of PEP8 regarding module-level constants is what I posted on the PR description:

• if it's a module level CONSTANT, that is, a true constant such as assignment to str, int, or tuple (?), then it's CAPITAL, so CONSTANT = 1
• if it's a module level variable, that is, assignment to something like a list, a class instance, then it should follow same rules as all other variables, that is, lower case snake... so dummy = Dummy() would be correct according to this.

What do you think? Is my interpretation wrong? If so, how to I interpret the test case suggestions in the issue?

[cdce8p]

DUMMY is a "constant" in pylint/Python terms and should be uppercased.

I agree with Daniel here. PEP 8 never differentiates between a constants and (true) constants. The pylint interpretation has been "consider every variable at module level a constant" for a long time. Changing it now would result in a massive breaking change that's just unreasonable.

Just an example. The change would result in over 7500+ invalid-name errors for Home-Assistant.

I'm strong -1.

[cdce8p]

DUMMY is a "constant" in pylint/Python terms and should be uppercased.

I agree with Daniel here. PEP 8 never differentiates between a constants and (true) constants. The pylint interpretation has been "consider every variable at module level a constant" for a long time. Changing it now would result in a massive breaking change that's just unreasonable.

Just an example. The change would result in over 7500+ invalid-name errors for Home-Assistant.

I'm strong -1.

[Pierre-Sassoulas]

Counter argument to not doing anything would be that invalid-name is unusable (the first thing I always do in a new project is to disable it). It's probably one of the most annoying thing when trying to use pylint. It's in pylint so it must be important, so you try to follow it at first... but then it does not make a lot of sense. Well we're not talking about #7305 there but of the more minor problem of module level "constant", so let's focus on that.

What we can't do do is make it even more annoying by increasing the constraints (7500+ new messages in a well maintained code base is just nuts). There's two way forward on this particular problem imo:

• keeping it like it is so those that actually use it do not have a breaking change (it's not very satisfying considering this check is one of the major pain point of pylint)
• Making the constraints laxer. Allowing both constant and variable style at the module level. Instead of trying to treat immutable and mutable differently: just treat them all with leniency. python does not have constants, we can't know, trust the user on this one.

I think it's clear what I would favor here 😄

[cdce8p]

Counter argument to not doing anything would be that invalid-name is unusable (the first thing I always do in a new project is to disable it). It's probably one of the most annoying thing when trying to use pylint.

I would disagree. In my personal experience it works fine as it is.

Clearly not everybody feels that way 😉 but that might rather be a thing that could be solved by config profiles. There was a small discussion about it in #3512.

• keeping it like it is so those that actually use it do not have a breaking change (it's not very satisfying considering this check is one of the major pain point of pylint)
• Making the constraints laxer. Allowing both constant and variable style at the module level. Instead of trying to treat immutable and mutable differently: just treat them all with leniency. python does not have constants, we can't know, trust the user on this one.

I would prefer the first option. For me it isn't so much weather or not it's a true constant but rather to know if a variable is defined at module level. Having the distinction between upper case and lower case does help with that.

Additionally, it adds a common style that, even if not perfect, everybody (who uses it regularly) understands. Relaxing the requirement will just lead to mixed cases which will require additional attention from reviewers. Especially for beginners, without experience in other languages, it won't be obvious when to use what and I suspect they will frequently use the wrong one.

[Pierre-Sassoulas]

The issue is that there is no consensus on what the style should be for module level variable, pep8 is lax about it:

• https://peps.python.org/pep-0008/#constants
• https://peps.python.org/pep-0008/#variable-annotations
  So I don't think pylint should take a stance about this, especially based on what we think is mutable / immutable which is another thing entirely.

The solution I would propose is to permit only consistent uppercase or consistent snake case, (or dunder casing, which is consistent snake case) but not anything else. We could add an option deactivated by default for that. The discussion about this is shattered in a dozen issues but I thought this was consensual.

[cdce8p]

So I don't think pylint should take a stance about this, especially based on what we think is mutable / immutable which is another thing entirely.

We don't do that. The difference is based on the scope a variable is defined in, e.g. module or function. IMO much easier to understand.

The solution I would propose is to permit only consistent uppercase or consistent snake case, (or dunder casing, which is consistent snake case) but not anything else. We could add an option deactivated by default for that. The discussion about this is shattered in a dozen issues but I thought this was consensual.

That's already possible with naming styles. https://pylint.pycqa.org/en/latest/user_guide/messages/convention/invalid-name.html#predefined-naming-styles For example, this should work

# pylintrc
const-naming-style=snake_case
variable-naming-style=snake_case

[Pierre-Sassoulas]

We don't do that. The difference is based on the scope a variable is defined in, e.g. module or function. IMO much easier to understand.

See #7808 (comment)

That's already possible with naming styles.

Well if you do const-naming-style=snake_case then you can't have const in UPPERCASE, right ? You could however create a complicated regex that match both. Maybe that's the easy fix.

[clavedeluna]

I've now updated this PR to only modify disallowed-name, not invalid-name.

[clavedeluna]

This could've been done in other ways, too, including integrating with _should_exempt_from_invalid_name above but I chose this for simplicity. Lmk if another implementation makes more sense.

[DanielNoord]

Makes sense to me, but I'd like the opinion of others as well.

[Pierre-Sassoulas]

LGTM ! I don't think we have to resolve the previous discussion to fix this issue and merge. We're going to have to do it regardless of the result of the discussion.

[Pierre-Sassoulas]

👍 good work merging those two

[Pierre-Sassoulas]

This was a bug, but it's going to make new messages appear so let's delay it to 2.16.0 as we advertise the patch version to only remove message in the doc: https://pylint.pycqa.org/en/latest/user_guide/installation/upgrading_pylint.html#upgrading-pylint

[coveralls]

Pull Request Test Coverage Report for Build 3708409930

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.0002%) to 95.445%

---

Totals
Change from base Build 3708337066:	0.0002%
Covered Lines:	17664
Relevant Lines:	18507

---

💛 - Coveralls

[clavedeluna]

any reason this can't go out with 2.16.0?

[Pierre-Sassoulas]

No but, I'm trying to wrap up 2.16.0 and focus on what is absolutely necessary (known unreleased issues).

[codecov]

Codecov Report

Merging #7808 (8e278c2) into main (92ec5d2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7808   +/-   ##
=======================================
  Coverage   95.46%   95.46%           
=======================================
  Files         176      176           
  Lines       18543    18544    +1     
=======================================
+ Hits        17702    17703    +1     
  Misses        841      841           

Impacted Files	Coverage Δ
pylint/checkers/base/name_checker/checker.py	99.23% <100.00%> (+<0.01%)	⬆️

[Pierre-Sassoulas]

Two nits and let's merge, good job!

[Pierre-Sassoulas]

I think it's high confidence, right ?

[github-actions]

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

This comment was generated for commit 8e278c2

[Pierre-Sassoulas]

Great !

The change was limited in scope in the following changes.