Skip to content

Change versioning scheme to be more robust - #57

Merged
JelleZijlstra merged 9 commits into
typeshed-internal:mainfrom
hauntsaninja:dev
Sep 14, 2022
Merged

JelleZijlstra merged 9 commits into
typeshed-internal:mainfrom
hauntsaninja:dev

Conversation

@hauntsaninja

Copy link
Copy Markdown
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 thread stub_uploader/get_version.py Outdated
Comment thread stub_uploader/get_version.py Outdated
Comment thread stub_uploader/get_version.py Outdated
Comment thread stub_uploader/get_version.py
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
Copy Markdown
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.

@hauntsaninja hauntsaninja Aug 26, 2022 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

@hauntsaninja hauntsaninja Sep 9, 2022 •

Copy link
Copy Markdown
Contributor Author

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

Comment thread stub_uploader/get_version.py Outdated
@hauntsaninja

Copy link
Copy Markdown
Contributor Author

Thanks for the review!!

hauntsaninja and others added 4 commits August 26, 2022 11:27
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
@hauntsaninja

Copy link
Copy Markdown
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

srittau commented Sep 14, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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