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

N804 disagrees with pylint on __new__ first argument's name for metaclass #10656

Closed
Amvoled opened this issue Mar 29, 2024 · 4 comments
Closed
Assignees
Labels
question Asking for support or clarification

Comments

@Amvoled
Copy link

Amvoled commented Mar 29, 2024

Here's an example:

class Alpha(type):
    def __new__(mcs, *args, **kwargs):
        return super().__new__(mcs, *args, **kwargs)

class Beta(type):
    def __new__(cls, *args, **kwargs):
        return super().__new__(cls, *args, **kwargs)
$ pylint meta.py
meta.py:7:4: C0204: Metaclass class method __new__ should have 'mcs' as first argument (bad-mcs-classmethod-argument)

$ ruff --select N meta.py
meta.py:2:17: N804 First argument of a class method should be named `cls`

To be fair I'm not sure if it's an issue with ruff or pylint, but they seem to both disagree on what to name the first argument of __new__ in a metaclass. Who's right ?

@charliermarsh
Copy link
Member

cls seems significantly more popular based on GitHub Code Search (37.2k files vs. 273 files), so I'd prefer to stick with what we have (though I know it's slightly inconvenient to have a difference across linters).

@charliermarsh charliermarsh added the question Asking for support or clarification label Mar 30, 2024
@charliermarsh charliermarsh self-assigned this Mar 30, 2024
@Pierre-Sassoulas
Copy link
Contributor

cls seems significantly more popular based on GitHub Code Search

How did you differentiate code concerning metaclasses and classes ? pylint has https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/bad-mcs-classmethod-argument.html (for metaclasses) but also https://pylint.readthedocs.io/en/stable/user_guide/messages/convention/bad-classmethod-argument.html (for classes). I think metaclasses methods are used way less frequently than class methods.

@jnrbsn
Copy link

jnrbsn commented Aug 23, 2024

@charliermarsh This is not accurate. In metaclasses, cls should only be used for the metaclass-equivalent of "instance methods" (i.e. methods that get passed, as their first argument, the class using the metaclass). For the metaclass-equivalent of "class methods" (like __new__ and __prepare__), the first argument passed to the method is the metaclass itself. In this case, pylint recommends mcs, and flake8-bugbear recommends metacls but also accepts mcs. flake8-bugbear implements this check via B902, which, in #3758, someone said is "Implemented as N804 and N805" in ruff, but that's not accurate because N804 incorrectly says you should use cls for __new__.

@eliegoudout
Copy link

eliegoudout commented Dec 21, 2024

I Completely agree with @jnrbsn. I always interpreted a class as an instance of a metaclass (are there even actual mechanical differences between classes and metaclasses except the ones due to subclassing type instead of object?). So logically, the (self, cls) from class context should be translated to (cls, mcs) in metaclass context, right?

However, I should point out something I find very strange from PEP 3115:

# The metaclass
class OrderedClass(type):

    # The prepare function
    @classmethod
    def __prepare__(metacls, name, bases): # No keywords in this case
        return member_table()

    # The metaclass invocation
    def __new__(cls, name, bases, classdict):
        # Note that we replace the classdict with a regular
        # dict before passing it to the superclass, so that we
        # don't continue to record member names after the class
        # has been created.
        result = type.__new__(cls, name, bases, dict(classdict))
        result.member_names = classdict.member_names
        return result

You can see that the author used different metacls and cls for __prepare__ and __new__ respectively, even though "cls.__new__() is a static method (special-cased so you need not declare it as such) that takes the class of which an instance was requested as its first argument"... I've not yet understood why.

So in the end, I do think Python doc suggests the use of mcs (or equivalent) for __new__ and classmethodin general, even though PEP3115 shows (a probably mistaken) inconsistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Asking for support or clarification
Projects
None yet
Development

No branches or pull requests

5 participants