Skip to content

Change versioning scheme to be more robust #57

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

Merged
merged 9 commits into from
Sep 14, 2022

Conversation

hauntsaninja
Copy link
Contributor

Currently sqlalchemy breaks typeshed CI basically whenever it releases a
new micro version. This would allow us to pin sqlalchemy to a tighter
version in METADATA.toml, while ensuring users who do
pip install types-sqlalchemy will still get the latest stub.

Please see the comments in compute_incremented_version about what we
attempt to guarantee (and the corresponding asserts in that function).

There's also some associated refactoring so that we can unit test logic
easily and so that we can lean a little more heavily on packaging to do
things for us.

Currently sqlalchemy breaks typeshed CI basically whenever it releases a
new micro version. This would allow us to pin sqlalchemy to a tighter
version in METADATA.toml, while ensuring users who do
`pip install types-sqlalchemy` will still get the latest stub.

Please see the comments in `compute_incremented_version` about what we
attempt to guarantee (and the corresponding asserts in that function).

There's also some associated refactoring so that we can unit test logic
easily and so that we can lean a little more heavily on `packaging` to do
things for us.
@JelleZijlstra JelleZijlstra requested a review from srittau August 21, 2022 03:20
Comment on lines +95 to +97
# We'll try to bump the fourth part of the release. So e.g. if our version_spec is
# 1.1, we'll release 1.1.0.0, 1.1.0.1, 1.1.0.2, ...
# But if our version_spec is 5.6.7.8, we'll release 5.6.7.8.0, 5.6.7.8.1, ...
Copy link
Member

Choose a reason for hiding this comment

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

Why change this behavior? For example, types-tzlocal currently has version 4.2.2, while tzlocal has version 4.2.

Copy link
Contributor Author

@hauntsaninja hauntsaninja Aug 26, 2022

Choose a reason for hiding this comment

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

I think it helps disambiguate "upstream version" from "types version". I've seen someone be confused by this and pin unnecessarily strictly.

E.g. if tzlocal released 4.2.1 that would still be compatible with 4.2 specified in METADATA.toml. But the correct corresponding version of types-tzlocal is still 4.2.2.

The issue comes about because the target version in METADATA.toml is hidden information to the average user. Very few packages use the fourth spot, so defaulting to the fourth spot in practice makes it clearer that the version isn't 1:1 with upstream versions since they are unlikely to overlap.

Is this a change you're open to?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine, but I'm concerned this could break types-tzlocal (and potentially others). I would like to see this changed in a separate PR to keep possible breakage per PR minimal.

Copy link
Contributor Author

@hauntsaninja hauntsaninja Sep 9, 2022

Choose a reason for hiding this comment

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

I'm curious if your concern can be turned into suggestions for tests. I just added a test_version_increment that runs determine_incremented_version on all distributions we publish. This checks that all invariants we assert are met for all currently distributed packages. Let me know if there's anything else that would increase confidence in these changes.

That aside, yes, I can comment this change out for now and raise exception in the change above

@hauntsaninja
Copy link
Contributor Author

Thanks for the review!!

@hauntsaninja
Copy link
Contributor Author

Bump, I think I've made all requested changes + added more tests

@srittau srittau dismissed their stale review September 14, 2022 09:27

No time to review.

@srittau
Copy link
Member

srittau commented Sep 14, 2022

I'm sorry, I have no time to review, but please merge if you and @JelleZijlstra are satisfied. And thanks for working on this!

@JelleZijlstra
Copy link
Contributor

Merging since I'm not sure @hauntsaninja has merge access here.

@JelleZijlstra JelleZijlstra merged commit 3a9497e into typeshed-internal:main Sep 14, 2022
@hauntsaninja hauntsaninja deleted the dev branch September 14, 2022 17:08
hauntsaninja added a commit to hauntsaninja/stub_uploader that referenced this pull request Sep 17, 2022
This change was removed from typeshed-internal#57, so as to make that PR more a
refactoring. The changes here accomplish two things:

First, allows us to change the specificity of how we pin versions in
typeshed while ensuring that users who install without pinning versions
still get the latest stub.

Second, incrementing the fourth position helps disambiguate "upstream
version" from "types version", reducing user confusion. See my comment
here: typeshed-internal#57 (comment)
srittau pushed a commit that referenced this pull request Sep 28, 2022
This change was removed from #57, so  The changes here accomplish two things:

First, allows us to change the specificity of how we pin versions in
typeshed while ensuring that users who install without pinning versions
still get the latest stub.

Second, incrementing the fourth position helps disambiguate "upstream
version" from "types version", reducing user confusion. See my comment
here: #57 (comment)
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.

3 participants