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

brain: Add __class_getitem__ to subprocess.Popen starting from Python 3.9 #882

Merged

Conversation

dbaty
Copy link
Contributor

@dbaty dbaty commented Jan 18, 2021

Type of Changes

Type
βœ“ πŸ› Bug fix

Related Issue

Closes pylint-dev/pylint#4034

Description

This fix is necessary for pylint to detect that subprocess.Popen is subscriptable, starting from Python 3.9 (see pylint-dev/pylint#4034).

pylint relies on the presence of __class_getitem__() to determine if a class is subcriptable. Starting from Python 3.9, subprocess.Popen implements this method (and is thus subscriptable):

$ python3.9
>>> import subprocess
>>> subprocess.Popen.__class_getitem__
<bound method GenericAlias of <class 'subprocess.Popen'>>

…thon 3.9

This is necessary for pylint to detect that `subprocess.Popen` is
subscriptable, starting from Python 3.9 (see pylint-dev/pylint#4034).

    $ python3.9
    >>> import subprocess
    >>> subprocess.Popen.__class_getitem__
    <bound method GenericAlias of <class 'subprocess.Popen'>>
@dbaty dbaty force-pushed the dbaty/subprocess_popen_class_getitem branch from dfb4daf to 294623e Compare January 18, 2021 12:06
@hippo91 hippo91 self-assigned this Jan 19, 2021
Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @dbaty!
It's all good for me.
Just left a comment dealing with textwrap.

@@ -147,6 +148,12 @@ def kill(self):
"py3_args": py3_args,
}
)
if PY39:
code += """
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use textwrap.dedent here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hippo91 : I don't think we can. dedent() removes all common leading whitespaces, which means that calling this:

textwrap.dedent(
    """
    def __class_getitem__(cls, item):
        pass
    """
)

... returns this string:

def __class_getitem__(cls, item):
    pass

... which looks like a function, whereas we want the code to be indented to denote a method. Of course we could use textwrap.indent() too, like this:

textwrap.indent(
    textwrap.dedent(
        """
        def __class_getitem__(cls, item):
            pass
        """
    ),
    ' ' * 4,
)

but I am not sure it's more readable. :)

I thus prefer the current version. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaty you are totally right!
Thanks for this explanation and this PR!

@hippo91 hippo91 merged commit 869b851 into pylint-dev:master Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False unsubscriptable-object positive with generic subprocess.Popen and python3.9
2 participants