Check that a name exists inside `enum.Enum` class' `members`. #8905

mbyrnepr2 · 2023-07-31T12:42:09Z

Type of Changes

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

Description

Check that a name exists inside an enum.Enum class' __members__ container before doing the invalid-name check.

Refs #7402

pylint/checkers/base/name_checker/checker.py

codecov · 2023-07-31T13:01:54Z

Codecov Report

Merging #8905 (03329f1) into main (6dac300) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8905   +/-   ##
=======================================
  Coverage   95.75%   95.75%           
=======================================
  Files         173      173           
  Lines       18652    18655    +3     
=======================================
+ Hits        17860    17863    +3     
  Misses        792      792

Files Changed	Coverage Δ
pylint/checkers/base/name_checker/checker.py	`98.59% <100.00%> (-0.02%)`	⬇️
pylint/checkers/utils.py	`95.93% <100.00%> (+0.02%)`	⬆️

mbyrnepr2 · 2023-08-02T16:00:35Z

pylint/checkers/base/name_checker/checker.py

@@ -486,12 +486,10 @@ def visit_assignname(  # pylint: disable=too-many-branches
        # Check names defined in class scopes
        elif isinstance(frame, nodes.ClassDef):
            if not list(frame.local_attr_ancestors(node.name)):


Wondering if this line can be removed 🤔

Unfortunately, removing this line caused #9765

mbyrnepr2 · 2023-08-03T12:11:06Z

There’s 2 warnings in the readthedocs output that are failing the ci but unsure which file it’s coming from exactly. Seems to be 3.0/index

Pierre-Sassoulas · 2023-08-04T06:56:37Z

I'm going to try to fix the readthedoc pipeline on main.

Pierre-Sassoulas · 2023-08-04T07:00:20Z

Well it was due to,

/home/docs/checkouts/readthedocs.org/user_builds/pylint/checkouts/8905/doc/whatsnew/3/3.0/index.rst:107: WARNING: Bullet list ends without a blank line; unexpected unindent.

/home/docs/checkouts/readthedocs.org/user_builds/pylint/checkouts/8905/doc/whatsnew/3/3.0/index.rst:112: WARNING: Bullet list ends without a blank line; unexpected unindent.

It was fixed by @jacobtylerwalls in #8921, so rebasing should work, thank you Jacob :)

mbyrnepr2 · 2023-08-04T11:19:00Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on music21: The following messages are now emitted:

invalid-name:
Class attribute name "registerOutputSubformatExtensions" doesn't conform to '([A-Za-z][A-Za-z0-9_]{2,30}|(._))$' pattern*
https://github.com/cuthbertLab/music21/blob/8baa27339109895cc4c55dd99cb0290e111fb02a/music21/converter/subConverters.py#L355

invalid-name:
Class attribute name "registerOutputSubformatExtensions" doesn't conform to '([A-Za-z][A-Za-z0-9_]{2,30}|(._))$' pattern*
https://github.com/cuthbertLab/music21/blob/8baa27339109895cc4c55dd99cb0290e111fb02a/music21/converter/subConverters.py#L411

invalid-name:
Class attribute name "registerOutputSubformatExtensions" doesn't conform to '([A-Za-z][A-Za-z0-9_]{2,30}|(._))$' pattern*
https://github.com/cuthbertLab/music21/blob/8baa27339109895cc4c55dd99cb0290e111fb02a/music21/converter/subConverters.py#L834

This comment was generated for commit 83a3503

These seem valid because the names are too long for the regex defined in that project's pylintrc:

import re
>>> re.match("([A-Za-z_][A-Za-z0-9_]{2,30}|(__.__))$", "registerOutputSubformatExtensions")
>>> 
>>> re.match("([A-Za-z_][A-Za-z0-9_]{2,30}|(__.__))$", "registerOutputSubformatExtensio")
<re.Match object; span=(0, 31), match='registerOutputSubformatExtensio'>
>>>

Pierre-Sassoulas

It would mean we never raise on an enum ? In the example with color and rgb the color is a three ints tuple and red/green/blue is simply an int maybe it's something to explore ?

mbyrnepr2 · 2023-08-04T20:59:24Z

@Pierre-Sassoulas in the context of the example you refer to, the message doesn't raise and that is the expected outcome since the members satisfy the checker and the attributes are not assigned values (hence they don't appear in the __members__ container). However, you need to also checkout the astroid branch pylint-dev/astroid#2263 to see it working correctly.

from enum import Enum

class AnEnum(Enum):
    apple: int
    PEAR: int = 42


print(AnEnum.__members__)
{'PEAR': <AnEnum.PEAR: 42>}

Let's add a lower-cased purple member to the example:

from enum import Enum

class Color(Enum):
    """Represents colors as (red, green, blue) tuples."""

    YELLOW      = 250, 250,   0
    KHAKI       = 250, 250, 125
    MAGENTA     = 250,   0, 250
    VIOLET      = 250, 125, 250
    CYAN        =   0, 250, 250
    AQUAMARINE  = 125, 250, 250
    purple      = 1, 2, 3

    # Class constant name "red" doesn't conform to UPPER_CASE naming style (invalid-name)
    # Class constant name "green" doesn't conform to UPPER_CASE naming style (invalid-name)
    # Class constant name "blue" doesn't conform to UPPER_CASE naming style (invalid-name)
    red: int
    green: int
    blue: int

    def __init__(self, red: int, green: int, blue: int) -> None:
        self.red   = red
        self.green = green
        self.blue  = blue

    @property
    def as_hex(self) -> str:
        """Get hex 'abcdef' representation for a color."""
        return f'{self.red:0{2}x}{self.green:0{2}x}{self.blue:0{2}x}'

example.py: 12:4: C0103: Class constant name "purple" doesn't conform to UPPER_CASE naming style (invalid-name)

Pierre-Sassoulas

Right my bad didn't see that the functional test was actually raising here. Could you add the rgb example as a functional test ? LGTM otherwise.

mbyrnepr2 · 2023-08-05T07:39:51Z

Unittests will fail now until pylint-dev/astroid#2263 is released.
About that - I currently have the astroid change set for 3.0 release but would you prefer that to be changed to 2.15.7 instead?

Pierre-Sassoulas

I liked your own functional test too, we can have both :) otherwise LGTM

Pierre-Sassoulas · 2023-08-05T15:14:34Z

Is it a bug fix il astroid ? If so we can backport it. If not let's release this in 3.0

mbyrnepr2 · 2023-08-07T11:16:23Z

Is it a bug fix il astroid ? If so we can backport it. If not let's release this in 3.0

It's a bug-fix in both astroid and Pylint (if I've understood you correctly :))
astroid fix: exclude non-members from __members__ (i.e: tomato: int)
pylint fix: check that the AssignName belongs to the __members__ of it's class (and not only check that the class is a subtype of enum.Enum.)

mbyrnepr2 · 2023-08-07T11:17:02Z

I liked your own functional test too, we can have both :) otherwise LGTM

No worries - less is more :)

Pierre-Sassoulas · 2023-08-07T12:20:23Z

Blocked by the releease of astroid 2.15.7 then ?

mbyrnepr2 · 2023-08-07T12:26:11Z

It's also blocked by #8897 since the code in that example when run with the Pylint code of the current MR crashes 😢

mbyrnepr2 · 2023-08-07T12:44:01Z

It's also blocked by #8897 since the code in that example when run with the Pylint code of the current MR crashes 😢

But I may have a fix for that shortly :)

…ontainer before doing the ``invalid-name`` check. Refs pylint-dev#7402

github-actions · 2023-09-23T19:49:17Z

🤖 Effect of this PR on checked open source code: 🤖

Effect on home-assistant:
The following messages are no longer emitted:

invalid-name:
Class constant name "_recoverable" doesn't conform to UPPER_CASE naming style
https://github.com/home-assistant/core/blob/6383cafeb9643266d7a7f6dc1ec4519b3775b58b/homeassistant/config_entries.py#L110

Effect on music21:
The following messages are now emitted:

invalid-name:
Class attribute name "registerOutputSubformatExtensions" doesn't conform to '([A-Za-z_][A-Za-z0-9_]{2,30}|(__.__))$' pattern*
https://github.com/cuthbertLab/music21/blob/f457a2ba52ea3f978d805cd52fa101dfb0dd8577/music21/converter/subConverters.py#L355
invalid-name:
Class attribute name "registerOutputSubformatExtensions" doesn't conform to '([A-Za-z_][A-Za-z0-9_]{2,30}|(__.__))$' pattern*
https://github.com/cuthbertLab/music21/blob/f457a2ba52ea3f978d805cd52fa101dfb0dd8577/music21/converter/subConverters.py#L411
invalid-name:
Class attribute name "registerOutputSubformatExtensions" doesn't conform to '([A-Za-z_][A-Za-z0-9_]{2,30}|(__.__))$' pattern*
https://github.com/cuthbertLab/music21/blob/f457a2ba52ea3f978d805cd52fa101dfb0dd8577/music21/converter/subConverters.py#L834

Effect on django:
The following messages are no longer emitted:

invalid-name:
Class constant name "do_not_call_in_templates" doesn't conform to UPPER_CASE naming style
https://github.com/django/django/blob/574ee4023e15cfb195833edfbaed353f8021c62f/django/db/models/enums.py#L85

This comment was generated for commit 03329f1

jacobtylerwalls · 2023-09-23T21:48:17Z

@mbyrnepr2 do you know why tests are passing without your astroid changes here? 🙏

mbyrnepr2 · 2023-09-24T05:59:58Z

I think the changes from astroid were pulled in from latest push? 2.15.7 astroid went out last month I think. There’s been a couple of different enum changes.

jacobtylerwalls · 2023-09-24T12:47:31Z

2.15.7 went out yesterday, and we haven't updated pylint to that or created a new astroid pre-release (but hoping to do so soon)

jacobtylerwalls · 2023-09-24T12:48:50Z

Ooh, Pierre is doing the pre-release right now!

Pierre-Sassoulas · 2023-09-24T13:12:44Z

We have both 2.15.7 and 3.0.0b0 released now, so it should be possible to merge on main then backport.

mbyrnepr2 commented Jul 31, 2023

View reviewed changes

pylint/checkers/base/name_checker/checker.py Outdated Show resolved Hide resolved