Experimental: Unify NamingSchemes. #3704

bukzor · 2020-06-21T07:42:09Z

Description

The current default pylint naming schemes only differ on three parameters:

characters allowed at beginning of name
characters allowed elsewhere
minimum length of name

Where this was not true was due to either bugs, oversight, or legacy
inertia; i's hard for me to tell the difference. Many odd
discrepancies among the naming schemes have been rectified, and this
change will bother users by calling out some of their bizarre naming
conventions, but all such issues are valid.

For churn considerations, I've not (yet?) modified the names of the
naming scheme objects although they've transitioned from being classes
to objects, and there's a couple other bits of odd naming in here for
the same reason. My aim was chiefly to show that this was possible, and
spur consideration of the idea.

A further improvement would be to set a "minimum length" for names in
the configuration, which should be checked apart from "naming scheme"
issues. This would greatly reduce the need for users to read and
understand regular expressions, and will make messages more actionable,
but also separating that additional resonsibility would further simplify
and rectify the naming schemes.

Type of Changes

	Type
	🐛 Bug fix
	✨ New feature
✓	🔨 Refactoring
	📜 Docs

bukzor · 2020-06-21T08:24:43Z

I don't understand these test failures. I'm unable to reproduce them reliably, and I notice they only happen when I run the whole test suite -- the tests alone don't fail. I also see that I sometimes get the same errors in master branch. Are there currently flaky tests in master?

The current default pylint naming schemes only differ on three parameters: 1. characters allowed at beginning of name 2. characters allowed elsewhere 3. minimum length of name Where this was not true was due to either bugs, oversight, or legacy inertia. It's hard for me to tell the difference. Many odd discrepancies among the naming schemes have been rectified, and this change will bother users by calling out some of their bizarre naming conventions, but all such issues are valid. For churn considerations, I've not (yet?) modified the names of the naming scheme objects although they've transitioned from being classes to objects, and there's a couple other bits of odd naming in here for the same reason. My aim was chiefly to show that this was possible, and spur consideration of the idea. A further improvement would be to set a "minimum length" for names in the configuration, which should be checked apart from "naming scheme" issues. This would greatly reduce the need for users to read and understand regular expressions, and will make messages more actionable, but also separating that additional resonsibility would further simplify and rectify the naming schemes.

coveralls · 2020-11-19T22:30:51Z

Coverage increased (+0.02%) to 90.839% when pulling edae48b on bukzor:unify-naming-schemes into 9a5e1b3 on PyCQA:master.

coveralls · 2020-11-19T22:30:51Z

Coverage increased (+0.02%) to 90.839% when pulling edae48b on bukzor:unify-naming-schemes into 9a5e1b3 on PyCQA:master.

coveralls · 2020-11-19T22:30:51Z

Coverage increased (+0.02%) to 90.839% when pulling edae48b on bukzor:unify-naming-schemes into 9a5e1b3 on PyCQA:master.

hippo91

@bukzor thanks a lot for this PR and sorry for the delay.
It is clearly an improvement in our naming scheme management.
However i'm a bit confused by the fact that, at this point, this PR prevents the user to use variable with only one character in its name...
By the way, i would like to have @PCManticore on this topic.

hippo91 · 2020-11-26T17:27:17Z

ChangeLog

+
+   1. increase the length of names to be more descriptive (recommended)
+   2. add strange-but-useful names to the good-names configuration
+   3. override the default regexen, via pylintrc (not recommended)


Why is that not recommended?

It's not recommended because even for those deeply familiar with regex (few) it is much too easy to make regex with unintended matches / exclusions.

hippo91 · 2021-01-02T10:52:49Z

pylint/checkers/base.py

+    name_template = r"[^\W\d_%s][^\W%s]%s"
+
+    def __init__(self, head_exclude: str, tail_exclude: str, min_length: int):
+        self.head_exclude = head_exclude


IMHO it could be interesting to add comments explaining what do you call head and tail. From what i understand, head is only one character long whereas tail is all the rest. I'm i right?

Yes, exactly. As in head and tail of a snake -- the tail is everything but the head.

pytest.ini

tests/conftest.py

hippo91 · 2021-01-04T18:07:04Z

tests/extensions/data/compare_to_zero.py

@@ -1,4 +1,4 @@
-# pylint: disable=literal-comparison,missing-docstring,misplaced-comparison-constant
+# pylint: disable=literal-comparison,missing-docstring,misplaced-comparison-constant,invalid-name


I find a little bit damageable if pylint is not compatible with constant name that are one letter long.
What do you think about it?

hippo91 · 2021-01-04T18:10:11Z

tests/messages/func_w0801.txt

-==input.func_w0801:3
-==input.w0801_same:3
+==input.func_w0801:4
+==input.w0801_same:4


Why is this change needed?

hippo91 · 2021-01-04T18:23:40Z

tests/functional/u/undefined_variable.rc

@@ -0,0 +1,2 @@
+[basics]
+good-names=i,j,k,ex,Run,_,_dt


Once again i think it is very risky to forbid using single character named variable...

DanielNoord · 2021-10-24T15:08:52Z

@bukzor Thanks for you initial contribution. I would like to add this to pylint and finish the work you started. Would you be okay with me creating a new PR based on the work you already did?

DanielNoord · 2022-04-01T21:06:48Z

I'm going to close this PR as it has become stale.

I looked at the changes this PR would introduce and have decided not to take them further myself. I think a real contribution would be finding a way to do the naming styles without having to compile 25+ regex patterns, but I'm not sure if that is even feasible.

bukzor · 2022-08-11T14:42:01Z

I intend to revive this work -- after a long haitus, I'm once again participaing in the open-source community.

Please advise:

Would you prefer I update this PR or start a fresh one?
Are there any of these changes that should be excluded?

DanielNoord · 2022-08-11T14:52:00Z

I intend to revive this work -- after a long haitus, I'm once again participaing in the open-source community.

Welcome back! 😄

Please advise:

Would you prefer I update this PR or start a fresh one?

Are there any of these changes that should be excluded?

I think a fresh one is probably better since many of the files here have now moved or significantly changed.

That said, I know there was some discussion about whether we should pursue the changes in this PR. I'm still sympathetic to simplifying this code, but I'm not sure how feasible the current approach still is.
We have now also have types that are only part of a specific style and are not defined on other styles, such as TypeVar.

bukzor · 2022-08-11T15:31:36Z

I intend to revive this work -- after a long haitus, I'm once again participaing in the open-source community.

Welcome back! 😄

Thank you!

Please advise:

Would you prefer I update this PR or start a fresh one?

Are there any of these changes that should be excluded?

I think a fresh one is probably better since many of the files here have now moved or significantly changed.

Roger.

That said, I know there was some discussion about whether we should pursue the changes in this PR. I'm still sympathetic to simplifying this code, but I'm not sure how feasible the current approach still is. We have now also have types that are only part of a specific style and are not defined on other styles, such as TypeVar.

I'm aware that the approach taken isn't strictly agreeable, but I do believe the goals are still relevant and attainable. Once I have something worth discussing I'll bother you again. Async text (i.e. github issues) isn't very conducive to debating abstract ideas such as hypothetical implementations, especially between strangers. How do you normally handle such complex communications?

DanielNoord · 2022-08-11T15:33:54Z

I'm aware that the approach taken isn't strictly agreeable, but I do believe the goals are still relevant and attainable. Once I have something worth discussing I'll bother you again. Async text (i.e. github issues) isn't very conducive to debating abstract ideas such as hypothetical implementations, especially between strangers. How do you normally handle such complex communications?

Normally we use issues to discuss significant proposals of change. It's not really perfect like you said, but it also helps narrow the scope of PRs which greatly helps with reviewing them. If the discussion in a PR is becoming too long it is often a good indication that it might try to do too much in one PR.

Pierre-Sassoulas · 2022-08-11T16:02:29Z

Hello @bukzor and welcome back !

(I started to work on this issue myself and lost the code some time ago so I had some ideas about it)

I think short variable name should be correctly checked as per hippo's comment. Also imo a split between a new check for variable names that are too small and variable names not respecting the name convention is required (using old_names so retro compatibility for all messages disabled with 'invalid-name' previousely are still disabled for both new message).

We usually use async text communication for everything as we're in various timezones and pylint is something I do on my free time / when convenient. So I don't want to schedule a call unless absolutely necessary. But I think it's possible to talk about the required steps and do them one by one with issues as Daniel said though.

bukzor · 2022-08-11T23:45:20Z

Hello @bukzor and welcome back !

Thank you :D

I think short variable name should be correctly checked as per hippo's comment.

Please expand on "correctly" here? My best guess is that you mean: One-character variables should be admitted by the name checker without error, by default.

Also imo a split between a new check for variable names that are too small and variable names not respecting the name convention is required ...

To clarify, you're advocating a split of the existing NameChecker into two new checkers? One for naming style, and one for naming length. The naming-style checker would disregard any length problems.

If so, my goal would be to add the two new checkers in a disabled-by-default state until you're ready, while leaving the old checker untouched, for now.

... (using old_names so retro compatibility for all messages disabled with 'invalid-name' previousely are still disabled for both new message).

I don't understand this bit. What is old_names? Do you mean that there would be two new error names that would be set to also disable themselves with disablement of invalid-name? I didn't know that was a feature of checkers.

We usually use async text communication for everything as we're in various timezones and pylint is something I do on my free time / when convenient. So I don't want to schedule a call unless absolutely necessary. But I think it's possible to talk about the required steps and do them one by one with issues as Daniel said though.

Got it! I will endevour to adopt this workflow. Thank you for including me :)

Pierre-Sassoulas · 2022-08-13T09:59:23Z

Please expand on "correctly" here? My best guess is that you mean: One-character variables should be admitted by the name checker without error, by default.

I was thinking that a name that is in the list of accepted name should be ok for both checks.

ab should be valid snake case but too short, and that aB should be invalid and too short, Ab should be valid PascalCase, but not ab, etc.. We would also have to define what we want for one letter name. i.e. A and a, is A Pascalcase or uppercase with underscore ? is a camelCase or snake_case ? Maybe both ?

snake_case name	A	a	Ab	ab	Abcd	abcd
invalid-name	yes	no	yes	no	yes	no
name-too-short	yes	yes	yes	yes	no	no

I realize that if we keep regex this is going to be hell so maybe no invalid name for name that are too short when the regex used is the default one (maybe someone crafted a masterful regex that handle short names properly somewhere) is acceptable:

snake_case name	A	a	Ab	ab	Abcd	abcd
invalid-name	no	no	no	no	yes	no
name-too-short	yes	yes	yes	yes	no	no

To clarify, you're advocating a split of the existing NameChecker into two new checkers? One for naming style, and one for naming length. The naming-style checker would disregard any length problems.

I think depending on what we decide for the problem above having one checker instead of two could be simpler but yes.

If so, my goal would be to add the two new checkers in a disabled-by-default state until you're ready, while leaving the old checker untouched, for now.

That could work well but add complexity, if we can refactor the existing checker it's probably less work overall. How would you do the change to the TypeVar check @DanielNoord ? Would a standalone checker with only TypeVar work ? Should we keep TypeVar "generic" ?

Do you mean that there would be two new error names that would be set to also disable themselves with disablement of invalid-name?

Yes, check this code: https://github.com/PyCQA/pylint/blob/main/pylint/checkers/base/docstring_checker.py#L64
Result here, it's possible to enable missing-function-docstring by using the old name : https://github.com/PyCQA/pylint/blob/main/tests/functional/u/use/use_symbolic_message_instead.py#L20

this supercedes pylint-dev#3704

bukzor force-pushed the unify-naming-schemes branch 6 times, most recently from b231639 to fa1f686 Compare June 21, 2020 08:15

bukzor force-pushed the unify-naming-schemes branch 3 times, most recently from 632dcc0 to 5b8f498 Compare November 19, 2020 17:09

bukzor force-pushed the unify-naming-schemes branch from 5b8f498 to edae48b Compare November 19, 2020 17:10

hippo91 self-assigned this Nov 26, 2020

hippo91 added the Work in progress label Nov 26, 2020

hippo91 requested changes Jan 4, 2021

View reviewed changes

hippo91 added Waiting on author Indicate that maintainers are waiting for a message of the author and removed Work in progress labels Jan 4, 2021

DanielNoord mentioned this pull request Oct 27, 2021

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

Closed

4 tasks

DanielNoord mentioned this pull request Dec 23, 2021

Add typing to brain_dataclasses pylint-dev/astroid#1292

Merged

1 task

Pierre-Sassoulas added this to the 2.13.0 milestone Dec 23, 2021

DanielNoord modified the milestones: 2.13.0, 2.14.0 Jan 10, 2022

DanielNoord closed this Apr 1, 2022

bukzor mentioned this pull request Aug 13, 2022

proposal: a simplified and generalized invalid-name config #7305

Open

bukzor added a commit to bukzor/pylint that referenced this pull request Aug 13, 2022

Proposal: Unified naming schemes

b87bc1e

this supercedes pylint-dev#3704

		@@ -1,4 +1,4 @@
		# pylint: disable=literal-comparison,missing-docstring,misplaced-comparison-constant
		# pylint: disable=literal-comparison,missing-docstring,misplaced-comparison-constant,invalid-name

		@@ -0,0 +1,2 @@
		[basics]
		good-names=i,j,k,ex,Run,_,_dt

Uh oh!

Experimental: Unify NamingSchemes. #3704

Experimental: Unify NamingSchemes. #3704

Uh oh!

Conversation

bukzor commented Jun 21, 2020

Description

Type of Changes

Uh oh!

bukzor commented Jun 21, 2020

Uh oh!

coveralls commented Nov 19, 2020

Uh oh!

coveralls commented Nov 19, 2020

Uh oh!

coveralls commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hippo91 left a comment

Choose a reason for hiding this comment

Uh oh!

hippo91 Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

bukzor Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

hippo91 Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

bukzor Aug 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hippo91 Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

hippo91 Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

hippo91 Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Oct 24, 2021

Uh oh!

DanielNoord commented Apr 1, 2022

Uh oh!

bukzor commented Aug 11, 2022

Uh oh!

DanielNoord commented Aug 11, 2022

Uh oh!

bukzor commented Aug 11, 2022

Uh oh!

DanielNoord commented Aug 11, 2022

Uh oh!

Pierre-Sassoulas commented Aug 11, 2022

Uh oh!

bukzor commented Aug 11, 2022

Uh oh!

Pierre-Sassoulas commented Aug 13, 2022

Uh oh!

Uh oh!

coveralls commented Nov 19, 2020 •

edited

Loading