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

Add protocol type BaseVersion #400

Closed
wants to merge 1 commit into from
Closed

Add protocol type BaseVersion #400

wants to merge 1 commit into from

Conversation

jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Feb 7, 2021

This allows mypy to know about all available properties when APIs return
any Version type.

For example, the pip project defines functions that return a
_BaseVersion instance. One example is:

https://github.com/pypa/pip/blob/bbf8466088655d22cd46b286c8f0b8150754c1d9/src/pip/_internal/metadata/base.py#L24

Downstream projects, such as pip-tools, use mypy to help verify correct
API use. pip-tools then calls methods on the _BaseVersion instance which
reports:

"_BaseVersion" has no attribute "is_prerelease"

By defining the protocol, pip and other libraries can use use its typing
information and it will be more useful to more projects.

@uranusjr
Copy link
Member

uranusjr commented Feb 8, 2021

IMO a better approach would be to provide a public BaseVersion protocol (and probably one for specifiers as well). Since LegacyVersion is being deprecated, code needing to support non-PEP-440 versions will have to invent something, and a protocol would help the process.

@jdufresne
Copy link
Contributor Author

I'm open to the suggestion, but a couple questions:

Using Protocol requires a dependency on typing_extension. It wasn't added to stdlib typing until 3.8. Is adding that dependency okay?

What do you see as the advantage over using a simple base class? IIUC, code that needs to support non-PEP-440 could inherit from BaseVersion just as easily as it could use a protocol, right? I could very well be missing something.

@uranusjr
Copy link
Member

I believe it’d work putting it behind if typing.TYPE_CHECKING so the dependency is not needed at runtime, only during typechecks.

Once LegacyVersion is dropped, _BaseVersion either needs to made into a protocol or a pure abstract base class (all methods are raise NotImplementedError). The latter provides no typecheck safety unless inherited from abc.ABC, which incurs a runtime penalty.

This allows mypy to know about all available properties when APIs return
any Version type.

For example, the pip project defines functions that return a
_BaseVersion instance. One example is:

https://github.com/pypa/pip/blob/bbf8466088655d22cd46b286c8f0b8150754c1d9/src/pip/_internal/metadata/base.py#L24

Downstream projects, such as pip-tools, use mypy to help verify correct
API use. pip-tools then calls methods on the _BaseVersion instance which
reports:

    "_BaseVersion" has no attribute "is_prerelease"

By defining the protocol, pip and other libraries can use use its typing
information and it will be more useful to more projects.
@jdufresne jdufresne changed the title Define the full interface on the _BaseVersion class Add protocol type BaseVersion Feb 11, 2021
@jdufresne
Copy link
Contributor Author

Sounds good. Thanks for explaining.

I implemented BaseVersion as a protocol in the latest revision.

if TYPE_CHECKING:
from typing import Protocol

class BaseVersion(Protocol):
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to type __init__() here to make sure implementations don't accidentally cause Liskov violations?

@brettcannon
Copy link
Member

Is this actually worth the effort? The deprecation for LegacyVersion says it's gone in the next major version and that's the version we are currently working on (I think dropping Python 2.7 made sure of that 😉 ).

So I would actually say we should not take this PR and instead remove LegacyVersion and then do the work for the various open issues we have related to LegacyVersion. What do others think?

@pradyunsg
Copy link
Member

I don't think we need this now, assuming we do accept #407. :)

@pradyunsg pradyunsg marked this pull request as draft February 28, 2021 14:29
@pradyunsg
Copy link
Member

(converted to draft, so that we don't merge this accidentally)

@jdufresne
Copy link
Contributor Author

I think #407 is the way to go. I'm closing this.

Thanks for opening the PR @pradyunsg !

@jdufresne jdufresne closed this Feb 28, 2021
@jdufresne jdufresne deleted the base-version branch February 28, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants