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

Define the location attribute on base class ProcessEnv #377

Merged
merged 1 commit into from Feb 8, 2021
Merged

Define the location attribute on base class ProcessEnv #377

merged 1 commit into from Feb 8, 2021

Conversation

jdufresne
Copy link
Contributor

Fixes errors when running mypy on a project's noxfile.py:

noxfile.py:42: error: "ProcessEnv" has no attribute "location"

Discovered while adding types pip's noxfile.py:
pypa/pip#9411

Copy link
Collaborator

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

I'm not that familiar with mypy, but location is a per instance attribute, not sure if there is a way to declare it like that.

@jdufresne
Copy link
Contributor Author

jdufresne commented Jan 23, 2021

@stsewd The syntanx used here is a type declaration. It does not not assign the attribute to the class (or even the instance). A true assignment is still required for that. Observe:

class MyClass:
    a: str

obj = MyClass()
obj.a
$ python blah.py
Traceback (most recent call last):
  File "/home/jon/blah.py", line 5, in <module>
    obj.a
AttributeError: 'MyClass' object has no attribute 'a'

The syntax is use for type declaration only so it makes mypy happy. Please let me know if I can provide additional detail.

Fixes errors when running mypy on a project's noxfile.py:

    noxfile.py:42: error: "ProcessEnv" has no attribute "location"

Discovered while adding types pip's noxfile.py:
pypa/pip#9411
@theacodes
Copy link
Collaborator

Looks good to me. Thanks, @jdufresne!

@theacodes theacodes merged commit d6a0ba5 into wntrblm:master Feb 8, 2021
@jdufresne jdufresne deleted the location branch February 8, 2021 01:36
@cjolowicz
Copy link
Collaborator

cjolowicz commented Apr 29, 2021

FTR this annotation is not strictly correct: PassthroughEnv does not have a location attribute, only VirtualEnv and CondaEnv do. And for these, mypy should be able to work it out from the type annotations on __init__.

The problem is that typical access is via session.virtualenv.location, where session.virtualenv is annotated as ProcessEnv. That's also the use case in pip's noxfile, AFAICS from the linked PR.

A simple fix would be the following:

class ProcessEnv:
    location: Optional[str] = None

This would result in a runtime change though, adding a class attribute on ProcessEnv. I can't see a simple way to solve this purely on the typing level. 🤔

Another possibility, maybe preferable, would be to keep the type annotation from this PR, but make it raise on PassthroughEnv:

class PassthroughEnv(ProcessEnv):
    @property
    def location(self) -> str:
        raise RuntimeError("PassthroughEnv has no location")

This would also have the advantage that client code does not need to check against None to pass mypy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants