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

Rewrite answer objects #306

Merged
merged 162 commits into from
Feb 6, 2023
Merged

Conversation

PerchunPak
Copy link
Member

@PerchunPak PerchunPak commented May 7, 2022

This PR rewrites Public API answer classes.

Breaking Changes & Migration Steps

  • __init__ signatures were significantly changed. If you used those, you can update to new __init__ generated by dataclasses, or use build method. Although it's not something that is public API.
  • Response classes no longer have child classes inside, look at status_response.py.
  • In Java response class, favicon was renamed to icon.
  • map attribute was renamed to map_name in Bedrock response class.
  • In Bedrock response class, players_online and players_max were renamed to players.online and players.max.
  • In Bedrock response class, version.version was renamed to version.name.

TODO

  • Rename base classes with Base prefix.
  • Move and write tests.
  • Actually, do we need tests?
  • Rewrite tests to use factory, currently there are too many duplicated code that can be avoided.
  • Write description for all attributes in classes, so users will not look for list of attributes with dir or looking in parents.
  • Update list of breaking changes.
  • Write migration steps (after PR's merge).

What to do after the deprecation period?

  • Look for TODO comments with 2023-08 mention, and do the instructions there.
  • Look for mcstatus.utils.deprecated usages with 2023-08 mention and remove such methods/properties.

@ItsDrike ItsDrike added type: feature New request or feature area: script Changes to the executable CLI script area: API Related to core API of the project state: WIP Work In Progress labels May 7, 2022
@PerchunPak PerchunPak changed the title Make MCServer as CrossPlatform solution Refactor answer objects May 7, 2022
@PerchunPak PerchunPak marked this pull request as draft May 7, 2022 20:32
@ItsDrike ItsDrike removed the area: script Changes to the executable CLI script label May 7, 2022
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.

This is a quick premature review just to point out some of the things which will need some changing. I just went through the changes really quickly and this is some of what I've noticed. There are some more little things which will also need to get addressed before this gets merged, but I didn't comment on those as this is still a work in progress PR and things will likely change anyway. I just wanted to point these things out, so they won't be forgotten about before the PR is marked as ready for review.

README.md Outdated Show resolved Hide resolved
mcstatus/server.py Outdated Show resolved Hide resolved
mcstatus/server.py Outdated Show resolved Hide resolved
mcstatus/server.py Outdated Show resolved Hide resolved
mcstatus/server.py Outdated Show resolved Hide resolved
mcstatus/mc_server.py Outdated Show resolved Hide resolved
@kevinkjt2000 kevinkjt2000 dismissed ItsDrike’s stale review February 6, 2023 16:59

All conversations marked as resolved from this review

@kevinkjt2000 kevinkjt2000 merged commit ffc5157 into py-mine:master Feb 6, 2023
@PerchunPak PerchunPak deleted the make-crosspatform-class branch February 6, 2023 21:17
@PerchunPak PerchunPak mentioned this pull request Feb 11, 2023
PerchunPak added a commit to PerchunPak/pinger-bot that referenced this pull request Mar 24, 2023
PerchunPak added a commit that referenced this pull request May 8, 2023
PerchunPak added a commit that referenced this pull request May 8, 2023
Tests are from #306 and code is dead since #515.
PerchunPak added a commit that referenced this pull request May 29, 2023
Tests are from #306 and code is dead since #515.
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 do-not-merge The PR can be reviewed but cannot be merged now state: waiting for author Waiting for author to address a review or respond to a comment type: rewrite Complete or partial rewrite of a part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants