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

Default TypeVar style for invalid-name too strict #5981

Closed
farmio opened this issue Mar 25, 2022 · 13 comments · Fixed by #5983
Closed

Default TypeVar style for invalid-name too strict #5981

farmio opened this issue Mar 25, 2022 · 13 comments · Fixed by #5983

Comments

@farmio
Copy link

farmio commented Mar 25, 2022

Bug description

HVACModeT = TypeVar("HVACModeT", "HVACControllerMode", "HVACOperationMode")
IPAddressT = TypeVar("IPAddressT")

would both lead to

C0103: Type variable name "IPAddressT" doesn't conform to predefined naming style (invalid-name)

Configuration

No response

Command used

pylint --jobs=0 *.py

Pylint output

C0103: Type variable name "HVACModeT" doesn't conform to predefined naming style (invalid-name)

Expected behavior

I would expect names beginning with multiple capital letters (like common abbreviations) to be valid TypeVar names, as they are for class names.

Pylint version

pylint 2.13.0
astroid 2.11.1
Python 3.9.7 (default, Nov 29 2021, 23:19:04)
[Clang 13.0.0 (clang-1300.0.29.3)]

OS / Environment

macOS 12.3

Additional dependencies

No response

@farmio farmio added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Mar 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Discussion 🤔 and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Mar 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.1 milestone Mar 25, 2022
@cdce8p
Copy link
Member

cdce8p commented Mar 25, 2022

Thanks for opening this one @farmio! I agree HVACModeT and IPAddress should be valid names. Saw something similar somewhere else already.

I opened #5983 to fix it. Should be included in the next release.

@cdce8p cdce8p self-assigned this Mar 25, 2022
@lyz-code
Copy link

I'm still encountering this issue with pylint 2.13.2

C0103 Type variable name "EntityType" doesn't conform to predefined naming style (invalid-name
pylint 2.13.2
astroid 2.11.2
Python 3.9.10 (main, Feb 15 2022, 11:52:08)
[GCC 8.3.0]

@DanielNoord
Copy link
Collaborator

Please see the discussion in #6003. We have explicitly chosen to disallow Type at the end of a TypeVar to distinguish it from a TypeAlias. EntityT should work!

@lyz-code
Copy link

Perfect, thank you ! :)

@sigma67
Copy link

sigma67 commented Sep 11, 2023

pylint still seems to complain if there is an underscore to mark it as internal, is that intended?

i.e. _IPAddressT

@DanielNoord
Copy link
Collaborator

I don't think it is, but this is not in my recent memory anymore.

@cdce8p @Pierre-Sassoulas Any opinion about this?

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Sep 13, 2023

There's a number of unused-x checks where adding a _ remove the message, as in "I know it's unused" (unused-variable for example cf https://pylint.readthedocs.io/en/stable/user_guide/configuration/all-options.html#dummy-variables-rgx, or unused-argument https://pylint.readthedocs.io/en/stable/user_guide/configuration/all-options.html#ignored-argument-names, unused imports too #8500). Its based on the loop variable _ that was becoming the standard for a voluntarily unused variable (like for _ in range(10)). I don't think it's a way to fast disable pylint on any message affecting a variable without using # pylint: disable=... (I don't think it should become that either).

@DanielNoord
Copy link
Collaborator

No, but we could allow a leading underscore in the default regex. That does make sense to me as I can see it as a common use case and something we might want to support by default without forcing users to overwrite the setting.

@cdce8p
Copy link
Member

cdce8p commented Sep 13, 2023

pylint still seems to complain if there is an underscore to mark it as internal, is that intended?

i.e. _IPAddressT

Are you sure? It works for me. Could you please check your config and see if there is a custom naming style defined somewhere?

from typing import TypeVar

_IPAddressT = TypeVar("_IPAddressT")
_IPAddressType = TypeVar("_IPAddressType")
test.py:3:0: C0103: Type variable name "_IPAddressType" doesn't conform to predefined naming style (invalid-name)

@Pierre-Sassoulas
Copy link
Member

Sorry I misunderstood the question really badly 😄 (I thought two consecutive capitalized letters in IP were supposed to raise). I can confirm Marc's result on pylint main.

@sigma67
Copy link

sigma67 commented Sep 13, 2023

Here is an example to reproduce (at least for me on pylint 2.17.5):

from typing import TypeVar

_IPAddressType = TypeVar("_IPAddressType", bound="IPAddress")

class IPAddress:
    pass

This works without error:

from typing import TypeVar

_IPAddressT = TypeVar("_IPAddressT", bound="IPAddress")

class IPAddress:
    pass

Edit: After re-reading your comments it seems this is intended behavior. In that case I guess I'd expect the error to be a bit more helpful by stating the expected naming style

@cdce8p
Copy link
Member

cdce8p commented Sep 13, 2023

Just to explain why _IPAddressType would be considered bad, the suffix Type should be used with TypeAliases, whereas for TypeVars it's usually preferred to use just the suffix T.

In that case I guess I'd expect the error to be a bit more helpful by stating the expected naming style

I would agree, however that's not as simple as it seems at first glance. Showing the full regex isn't helpful either probably and explaining would be too verbose. The also doesn't necessarily work if users choose a custom regex or naming style.

@sigma67
Copy link

sigma67 commented Sep 14, 2023

If you check the docs for invalid-name it's all there, it seems. Thanks for being patient with me.

https://pylint.pycqa.org/en/latest/user_guide/messages/convention/invalid-name.html#predefined-naming-patterns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants