8000 Make all our dataclasses frozen by PerchunPak · Pull Request #743 · py-mine/mcstatus · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make all our dataclasses frozen #743

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 11, 2024
Merged

Make all our dataclasses frozen #743

merged 2 commits into from
Feb 11, 2024

Conversation

PerchunPak
Copy link
Member

This fixes some of the errors that pyright outputs after an update:

/home/runner/work/mcstatus/mcstatus/mcstatus/status_response.py
  /home/runner/work/mcstatus/mcstatus/mcstatus/status_response.py:109:5 - error: "players" overrides symbol of same name in class "BaseStatusResponse"
    Variable is mutable so its type is invariant
      Override type "JavaStatusPlayers" is not the same as base type "BaseStatusPlayers" (reportIncompatibleVariableOverride)
  /home/runner/work/mcstatus/mcstatus/mcstatus/status_response.py:110:5 - error: "version" overrides symbol of same name in class "BaseStatusResponse"
    Variable is mutable so its type is invariant
      Override type "JavaStatusVersion" is not the same as base type "BaseStatusVersion" (reportIncompatibleVariableOverride)
  /home/runner/work/mcstatus/mcstatus/mcstatus/status_response.py:153:5 - error: "players" overrides symbol of same name in class "BaseStatusResponse"
    Variable is mutable so its type is invariant
      Override type "BedrockStatusPlayers" is not the same as base type "BaseStatusPlayers" (reportIncompatibleVariableOverride)
  /home/runner/work/mcstatus/mcstatus/mcstatus/status_response.py:154:5 - error: "version" overrides symbol of same name in class "BaseStatusResponse"
    Variable is mutable so its type is invariant
      Override type "BedrockStatusVersion" is not the same as base type "BaseStatusVersion" (reportIncompatibleVariableOverride)

This is probably a breaking change (you can't change the values of returned objects anymore), although we already have a breaking change (#682) in our master, so it is not a big deal I think.

@PerchunPak PerchunPak added type: enhancement Improvement to an existing feature area: API Related to core API of the project labels Feb 5, 2024
ItsDrike
ItsDrike requested changes Feb 7, 2024
View reviewed changes
Copy link
Member
@ItsDrike ItsDrike left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we do have a breaking change in master, however it's just a deprecation removal, the change itself was made a while ago and went through a full deprecation period by now.

This, in it's current state, would be a breaking change without any deprecation. However, there's no easy way to add deprecations here (it wouldn't be impossible, there's descriptors, but it would be a huge and complex mess, probably with a lot of type-ignores),

That said, I can't imagine this affecting too many people, as there shouldn't really be any need to modify the data from the responses. Because of that, I don't mind it hugely, I just wanted to mention this here, and it should also be made very clear in the release notes.

ItsDrike
ItsDrike approved these changes Feb 7, 2024
View reviewed changes
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.

Assuming we're fine with the breaking change here, this can be merged.

@PerchunPak PerchunPak enabled auto-merge (squash) February 11, 2024 13:53
@PerchunPak PerchunPak merged commit fbc826d into master Feb 11, 2024
@PerchunPak PerchunPak deleted the frozen-dataclasses branch February 11, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItsDrike ItsDrike ItsDrike approved these changes

Assignees
No one assigned
Labels
area: API Related to core API of the project type: enhancement Improvement to an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PerchunPak @ItsDrike
0