Create a `TypeVar` style for `invalid-name` #5894

DanielNoord · 2022-03-11T09:07:45Z

Add yourself to CONTRIBUTORS if you are a new contributor.
Write a good description on what the PR does.

Type of Changes

	Type
✓	✨ New feature
✓	🔨 Refactoring

Description

Not sure if we want to do this, but:
After merging #5825, we can merge this. It only adds the necessary boilerplate for typevar name checking, but uses the pattern as set for classes.
This makes the diff of #5221 even smaller and should help getting that merged asap after #5825 has been merged.

Closes #3401.

coveralls · 2022-03-11T09:16:42Z

Pull Request Test Coverage Report for Build 1986086805

27 of 27 (100.0%) changed or added relevant lines in 2 files are covered.
321 unchanged lines in 34 files lost coverage.
Overall coverage increased (+0.06%) to 94.093%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/newstyle.py	1	96.23%
pylint/checkers/spelling.py	1	76.41%
pylint/checkers/strings.py	1	93.78%
pylint/extensions/comparetozero.py	1	97.14%
pylint/extensions/emptystring.py	1	96.88%
pylint/extensions/typing.py	1	97.08%
pylint/pyreverse/mermaidjs_printer.py	1	98.18%
pylint/testutils/pyreverse.py	1	95.0%
pylint/checkers/deprecated.py	2	97.67%
pylint/checkers/design_analysis.py	2	98.91%

Totals
Change from base Build 1969334727:	0.06%
Covered Lines:	15196
Relevant Lines:	16150

💛 - Coveralls

cdce8p · 2022-03-11T15:07:09Z

pylintrc

+# Naming style used for type variables. Default is PascalCase.
+typevar-naming-style=PascalCase


I believe this was the most recent discussion we had: Should we add a typevar-naming-style add all or only have a typevar-rgx option?

The arguments so far

If we say the default is PascalCase, it will be difficult to explain why PathType is invalid and PathT valid when both are PascalCase.

It's unclear what the regex patterns for the other styles should be and how they should interact with the _co and _contra suffixes.

Having only the the rgx option (with default) would still allow for customizability if wanted.

What should be done in similar cases, e.g. for ParamSpec names or Type aliases?

Changing it should be possible if we modify _create_naming_rules and _create_naming_options to allow for rgx only options.

I was hesitant to do this at first, but turned out to be relatively easy to implement. I have pushed an iteration which does this to this branch. This basically supersedes #5221. I'm also fine with pushing it to that branch to keep the discussion in one place, but perhaps a relatively clean slate is helpful here.

cdce8p

This looks like the solution I had in mind. 👍🏻
I think it's a good idea to do the changes here and not in the old PR.

ChangeLog

doc/user_guide/options.rst

pylint/checkers/base.py

cdce8p · 2022-03-11T23:57:34Z

pylintrc

+# Regular expression which can overwrite the naming style set by typevar-naming-style.
+# If left empty, type variables will be checked with the set naming style.
+typevar-rgx=


That doesn't work. If pattern is empty, it will match an empty pattern.
https://github.com/PyCQA/pylint/blob/d01e5e167834097ff7e5bf26a069aa1339669183/pylint/config/option.py#L102

I checked. If it is empty the setting will be None (I think this is due to how the config file is parsed). The following lines make sure that the pattern is only picked up if the pattern is set. (Note that this is also how this works for all other patterns).

custom_regex = getattr(self.config, custom_regex_setting_name, None) if custom_regex is not None: regexps[name_type] = custom_regex

An empty setting will be re.compile('') and thus overwrite regexps[name_type]. That isn't bad, we just can't leave it empty in pylintrc. Otherwise TypeVar names in pylint itself won't be checked. That's similar to the other regex settings in pylintrc, eg. class-rgx.

Alternatively, an possibly my preference would be the comment out that line. Similar to #class-const-rgx=.

Ah I understand why I was mistaken. The functional test don't actually use this pylintrc so they passed even with this uncommented.

Commented it out!

tests/checkers/unittest_base.py

doc/whatsnew/2.13.rst

Co-authored-by: Marc Mueller <[email protected]>

cdce8p · 2022-03-13T22:15:33Z

doc/user_guide/options.rst

+
+The following type of names are checked with a predefined pattern:
+
+- TypeVars. Accepted names include: `T`, `TypeVar`, `_CallableT`, `_T_co`.


I wonder if a table would make sense here? With Name type, Good names, and Bad names.

I would prefer not to suggest TypeVar. True it's valid, but it shouldn't be used IMO. Let's use something like MyClassT to highlight PascalCase. For bad names, we should definitely mention something like T_Class, and MyClassType.

Some other names from the tests: AnyStr, DeviceTypeT, DeviceType, CALLABLE_T.

pylint/checkers/base.py

cdce8p · 2022-03-13T22:34:40Z

pylintrc

+# Regular expression which can overwrite the naming style set by typevar-naming-style.
+# If left empty, type variables will be checked with the set naming style.
+typevar-rgx=


An empty setting will be re.compile('') and thus overwrite regexps[name_type]. That isn't bad, we just can't leave it empty in pylintrc. Otherwise TypeVar names in pylint itself won't be checked. That's similar to the other regex settings in pylintrc, eg. class-rgx.

Alternatively, an possibly my preference would be the comment out that line. Similar to #class-const-rgx=.

cdce8p · 2022-03-13T22:40:30Z

tests/functional/t/typevar_naming_style_default.py

+GoodNameWithoutContra = TypeVar(  # [typevar-name-incorrect-variance]
+    "GoodNameWithoutContra", contravariant=True
+)
+GoodNameT_co = TypeVar("GoodNameT_co", covariant=True)
+GoodNameT_contra = TypeVar("GoodNameT_contra", contravariant=True)
+GoodBoundNameT = TypeVar("GoodBoundNameT", bound=int)
+GoodBoundNameT_co = TypeVar(  # [typevar-name-incorrect-variance]
+    "GoodBoundNameT_co", bound=int
+)
+GoodBoundNameT_contra = TypeVar(  # [typevar-name-incorrect-variance]
+    "GoodBoundNameT_contra", bound=int
+)
+GoodBoundNameT_co = TypeVar("GoodBoundNameT_co", bound=int, covariant=True)
+GoodBoundNameT_contra = TypeVar("GoodBoundNameT_contra", bound=int, contravariant=True)
+
+GoodBoundNameT_co = TypeVar("GoodBoundNameT_co", int, str, covariant=True)
+GoodBoundNameT_co = TypeVar(  # [typevar-name-incorrect-variance]
+    "GoodBoundNameT_co", int, str, contravariant=True
+)
+GoodBoundNameT_contra = TypeVar("GoodBoundNameT_contra", int, str, contravariant=True)
+GoodBoundNameT_contra = TypeVar(  # [typevar-name-incorrect-variance]
+    "GoodBoundNameT_contra", int, str, covariant=True
+)


Can we remove some of the GoodName TypeVars here? We already have dedicated tests for the typevar-name-incorrect-variance so I don't think we need to test this again here.

Basically just keep

# PascalCase names with prefix GoodNameT = TypeVar("GoodNameT") _T = TypeVar("_T") _GoodNameT = TypeVar("_GoodNameT") __GoodNameT = TypeVar("__GoodNameT") GoodNameT_co = TypeVar("GoodNameT_co", covariant=True) GoodNameT_contra = TypeVar("GoodNameT_contra", contravariant=True)

tests/functional/t/typevar_naming_style_default.py

Pierre-Sassoulas

Looks good !

Co-authored-by: Marc Mueller <[email protected]>

doc/user_guide/options.rst

DanielNoord added Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow labels Mar 11, 2022

DanielNoord added this to the 2.13.0 milestone Mar 11, 2022

DanielNoord force-pushed the typevar-boiler branch from fc94327 to 4abbdf8 Compare March 11, 2022 09:15

DanielNoord force-pushed the typevar-boiler branch from 4abbdf8 to fb0fe5d Compare March 11, 2022 14:40

DanielNoord requested a review from cdce8p March 11, 2022 14:40

cdce8p reviewed Mar 11, 2022

View reviewed changes

Create a TypeVar style for invalid-name

b0fe891

DanielNoord force-pushed the typevar-boiler branch from fb0fe5d to b0fe891 Compare March 11, 2022 15:37

DanielNoord changed the title ~~Add TypeVar name checking boilerplate~~ Create a TypeVar style for invalid-name Mar 11, 2022

DanielNoord added 2 commits March 11, 2022 16:45

Textual changes

0edcdef

Fix tests

58fd897

DanielNoord requested a review from cdce8p March 11, 2022 16:06

cdce8p requested changes Mar 12, 2022

View reviewed changes

DanielNoord commented Mar 12, 2022

View reviewed changes

doc/whatsnew/2.13.rst Outdated Show resolved Hide resolved

DanielNoord and others added 3 commits March 12, 2022 09:39

Apply suggestions from code review

e9da6f0

Co-authored-by: Marc Mueller <[email protected]>

Improve help message

93da592

Review

ea087ab

DanielNoord mentioned this pull request Mar 12, 2022

Fix invalid-name for TypeVar and add typevar-name-missing-variance checker #5221

Closed

4 tasks

Add doc

49f9174

DanielNoord requested a review from cdce8p March 12, 2022 09:21

cdce8p requested changes Mar 13, 2022

View reviewed changes

Pierre-Sassoulas approved these changes Mar 14, 2022

View reviewed changes

DanielNoord and others added 4 commits March 14, 2022 21:00

Apply suggestions from code review

527e283

Co-authored-by: Marc Mueller <[email protected]>

Reformatted to table

6bd9b6a

Update tests

bb4617b

Comment tout

57ca521

DanielNoord requested a review from cdce8p March 14, 2022 20:11

DanielNoord commented Mar 15, 2022

View reviewed changes

doc/user_guide/options.rst Outdated Show resolved Hide resolved

Update doc/user_guide/options.rst

cb78be4

Pierre-Sassoulas merged commit fd91d04 into pylint-dev:main Mar 24, 2022

DanielNoord deleted the typevar-boiler branch March 24, 2022 15:32

niklasmohrin mentioned this pull request Mar 28, 2022

Fix internal server errors on requests with unexpected parameters e-valuation/EvaP#1721

Merged

mina86 mentioned this pull request May 13, 2022

fuzzer: introduce fuzzer runner Near-One/nayduck#28

Merged

243f6a8885a308d313198a2e037 mentioned this pull request Sep 11, 2022

Fix typevar-rgx description in pylintrc #7455

Merged

3 tasks

		# Naming style used for type variables. Default is PascalCase.
		typevar-naming-style=PascalCase


		The following type of names are checked with a predefined pattern:

		- TypeVars. Accepted names include: `T`, `TypeVar`, `_CallableT`, `_T_co`.

Uh oh!

Create a TypeVar style for invalid-name #5894

Create a TypeVar style for invalid-name #5894

Uh oh!

Conversation

DanielNoord commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of Changes

Description

Uh oh!

coveralls commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1986086805

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdce8p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Create a `TypeVar` style for `invalid-name` #5894

Create a `TypeVar` style for `invalid-name` #5894

DanielNoord commented Mar 11, 2022 •

edited

Loading

coveralls commented Mar 11, 2022 •

edited

Loading