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

Rename status_response to responses #687

Merged
merged 15 commits into from
Jun 3, 2024

Conversation

PerchunPak
Copy link
Member

As we have #682, I thought it would be a nice idea to move 90337a7 into separate PR and release it with #682 in v12.

#536 shouldn't have any breaking (non-deprecatable) change, including 90337a7. I will also remove 90337a7 from #536 later.

@PerchunPak PerchunPak added type: enhancement Improvement to an existing feature area: API Related to core API of the project labels Dec 11, 2023
@PerchunPak PerchunPak self-assigned this Dec 11, 2023
@ItsDrike
Copy link
Member

ItsDrike commented Dec 11, 2023

This should go through deprecation though, we shouldn't make this kind of major change lightly. Even though v12 will be a major release, and breaking changes are technically allowed, this might hit a lot of people who are updating and I'd rather see this in v13 with some more deprecation time, or we could postpone v12 and submit it now, in a new minor release, with at least 2 months deprecation time, and release v12 only then.

If we would go the postponing route, #682 would be blocked until that

Copy link
Contributor

@kevinkjt2000 kevinkjt2000 left a comment

Choose a reason for hiding this comment

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

@PerchunPak An older file was moved because enforces_secure_chat is missing in attributes.

@ItsDrike there is a DeprecationWarning; should it mention v12/v13 as a removal timeline?

@ItsDrike
Copy link
Member

ItsDrike commented Dec 12, 2023

there is a DeprecationWarning; should it mention v12/v13 as a removal timeline?

Oh yeah, how did I miss that? lol, guess I'm blind. Anyway, it depends on how quickly do we want to push any pending breaking changes. If we do v12, it will block #682 until the end of the deprecation period here. Since the original deprecations in place used dates and not versions though, and that date there was 2023-11 (iirc), we should probably push a v12 now with those deprecations removed as a breaking change, and have this deprecated until v13.

This change itself can be pushed in v12, but also in any minor v12 release, like 12.1.0, as we don't consider introduction of deprecations as a breaking change, only their removal (at least as far as I'm aware, we don't actually have a versioning policy/guidelines page written out. We might want to do something like: https://mcproto.readthedocs.io/en/stable/pages/version_guarantees/).

@PerchunPak
Copy link
Member Author

An older file was moved because enforces_secure_chat is missing in attributes.

Yeah, accidentally copied from the wrong branch. Will fix it quiet later.

If we do v12, it will block #682 until the end of the deprecation period here

I would like to merge this together with #682 and release both PRs in v12, don't understand what blocks us from merging #682.

at least as far as I'm aware, we don't actually have a versioning policy/guidelines page written out

I thought we just strictly follow https://semver.org.

@ItsDrike
Copy link
Member

ItsDrike commented Dec 12, 2023

I thought we just strictly follow https://semver.org/.

Semver doesn't contain anything about deprecations, it just mentions that breaking changes should be done as a major release. Is deprecation a breaking change though? I'd say no, but some libs consider it as such. That's what we should establish in the versioning guarantees (see the mcproto docs link).

I would like to merge this together with #682 and release both PRs in v12, don't understand what blocks us from merging #682.

We can release both in v12, we just can't mark the deprecation period as "until v12", since I'd like to see at least 2 months there, and #682 is a breaking change, as it's removing a deprecation, so the code would actually end up broken, which forces us to make a major release once merged, i.e. we'd have to release v12. That's why these changes should be deprecated with a message saying "until v13", so in v13, the deprecations introduced here will be removed.

i.e.:

-warnings.warn("`mcstatus.status_response` is deprecated, use `mcstatus.responses` instead", DeprecationWarning, stacklevel=2)
+warnings.warn("`mcstatus.status_response` is deprecated, and will be removed in mcstatus v13, use `mcstatus.responses` instead", DeprecationWarning, stacklevel=2)

@PerchunPak
Copy link
Member Author

Oh, yeah, I forgot that I mostly just blindly cherry-picked commit. Will fix it as well as other changes.

@PerchunPak
Copy link
Member Author

Done. I also used 2024-3 instead of v13, as we everywhere else use dates instead of versions for deprecations.

@kevinkjt2000 kevinkjt2000 dismissed their stale review January 1, 2024 15:39

Looks like a newer version of status_response was copied

Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

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

Needs merge confict resolution, and the deprecation date should be bumped to at least 2024-05, or maybe even later

@PerchunPak PerchunPak force-pushed the rename-status-response-to-responses branch from 1e260bc to 030efae Compare February 19, 2024 19:02
@PerchunPak PerchunPak added status: needs review Author is waiting for someone to review and approve status: stale Has had no activity for a while labels May 5, 2024
@ItsDrike
Copy link
Member

I suppose this can be merged, though it'll definitely need bumping the deprecation dates. (Unless there is something else that this is waiting on, which I'm not aware of?)

@PerchunPak PerchunPak merged commit c2209c4 into master Jun 3, 2024
11 checks passed
@PerchunPak PerchunPak deleted the rename-status-response-to-responses branch June 3, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Related to core API of the project status: needs review Author is waiting for someone to review and approve status: stale Has had no activity for a while type: enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants