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

Use more descriptive type than _BaseVersion #9567

Closed
wants to merge 1 commit into from
Closed

Use more descriptive type than _BaseVersion #9567

wants to merge 1 commit into from

Conversation

jdufresne
Copy link
Contributor

Using _BaseVersion does not allow mypy to know about the Version methods
and properties. For example: .is_prerelease. By using the more
descriptive type, mypy is able to know about these and use them to
verify types.

This is the approach taken by the upstream packaging project. See:
https://github.com/pypa/packaging/blob/20.9/packaging/version.py

Beyond the internal benefit, this will help downstream projects such as
pip-tools which use pip with mypy:
https://github.com/jazzband/pip-tools/blob/5.5.0/piptools/exceptions.py#L20

Currently, the above fails mypy checks with the error:

"_BaseVersion" has no attribute "is_prerelease"

Using _BaseVersion does not allow mypy to know about the Version methods
and properties. For example: .is_prerelease. By using the more
descriptive type, mypy is able to know about these and use them to
verify types.

This is the approach taken by the upstream packaging project. See:
https://github.com/pypa/packaging/blob/20.9/packaging/version.py

Beyond the internal benefit, this will help downstream projects such as
pip-tools which use pip with mypy:
https://github.com/jazzband/pip-tools/blob/5.5.0/piptools/exceptions.py#L20

Currently, the above fails mypy checks with the error:

    "_BaseVersion" has no attribute "is_prerelease"
@pfmoore
Copy link
Member

pfmoore commented Feb 7, 2021

I hate to say it, but this is precisely the sort of thing I was concerned with in this comment on #9279. I understand that this change would have potential benefits internally for pip, as well, but that's secondary here, IMO. What matters to me is that we don't start treating pip's internals as something we should maintain for external users who are (for whatever reason) disregarding the warnings we give to not rely on pip's internals.

In addition, by depending directly on packaging.LegacyVersion, we'll need to change this again when packaging removes legacy versions completely. So this adds churn for pip, for little gain.

I'm -1 on this change, I'm afraid. Sorry if that seems harsh.

@jdufresne
Copy link
Contributor Author

Okay, no problem.

I opened a PR in packaging to try to improve the situation there. Maybe that will work out and obviate this need for me.

I never meant to add maintenance to the pip team. By volunteering with this and other PRs, I was hoping to show good will and take on any perceived maintenance work. I don't expect (or want) the pip team to change their API goals to accommodate pip-tools. So to be clear, I'm not looking for or asking for a change in pip's policy. I saw this as an easy, low risk, neutral change that made no new guarantees about an API.

@jdufresne jdufresne closed this Feb 7, 2021
@jdufresne jdufresne deleted the base-version branch February 7, 2021 23:16
@pfmoore
Copy link
Member

pfmoore commented Feb 8, 2021

Thanks - to be clear, I appreciate your help in improving things within pip. I'm not particularly familiar with type annotations, so having someone with more familiarity offer help is great, and please don't think I was suggesting otherwise.

For the record, my reservations were twofold. On the one hand, it wasn't obvious to me from the PR what (if any) types of issue this change would catch that we weren't picking up already, and on the other hand, I very much don't want to introduce extra dependencies on the LegacyVersion class, as I know it's slated for removal. Your PR for packaging, now that I've taken a look at it, seems like a better approach.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants