mcstatus icon indicating copy to clipboard operation
mcstatus copied to clipboard

Rewrite answer objects

Open PerchunPak opened this issue 2 years ago • 11 comments

This rewrites answer classes in Public API.

Current Breaking Changes:

  • Validation (from PingResponse) is now raising TypeError instead of ValueError if the given type was not correct.

Migration steps

  • TODO. It will be written when PR is merged. Now it is very fast changes.

TODO

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

do-not-merge label

This PR is too long not reviewed, so after fast conversation in discord, we thought about add do-not-merge label and bump date for removing deprecations right before merging this PR.

Deprecation period will be ~6 months after the merge.

PerchunPak avatar May 07 '22 16:05 PerchunPak

Not To Forget: Discuss and make backwards competitive.

PerchunPak avatar May 08 '22 12:05 PerchunPak

We should also consider adding both java and bedrock terms as properties in the base response classes.

Java terms slight archaic, I saw many MOTD plugins and all of them never called MOTD as description. I'm sure, will be enough just wrote how was renamed fields in change-log and merge-guide.

PerchunPak avatar May 08 '22 18:05 PerchunPak

The only problem of my backport is different signature in __init__ method. I tried do it with typing.overwrite, but it only produce more problems.

PerchunPak avatar May 11 '22 07:05 PerchunPak

Maybe I made too many parametrizing, because at now in master there is 159 tests, and in this PR 425.

PerchunPak avatar May 28 '22 08:05 PerchunPak

I hope no one (@ItsDrike, @kevinkjt2000) forgot about this PR, because last my reminder was in discord, two weeks ago. And no activity since it.

PerchunPak avatar Jun 16 '22 17:06 PerchunPak

AFAIK the PR is still awaiting the review from @kevinkjt2000, I will not request any more changes/approve the PR until that happens.

ItsDrike avatar Jun 16 '22 18:06 ItsDrike

@PerchunPak My life has been super busy lately. I reviewed approximately half of this 2 weeks ago. At the earliest, I will finally have time on Wednesday evening to finish reviewing.

kevinkjt2000 avatar Jun 20 '22 15:06 kevinkjt2000

Looks like last commits didn't run any CI, is this a bug? I also found one, when ran tests while resolving merge conflict.

EDIT: Runs now, maybe just logs was deleted.

PerchunPak avatar Jul 05 '22 16:07 PerchunPak

I just noticed that 2022-08 is coming soon. Of course even if #306 will be merged now, half a month isn't a very long time to delete the deprecations. What will be the new time?

PerchunPak avatar Jul 17 '22 13:07 PerchunPak

Adding do-not-merge label, as discussed on discord.

This PR can still be reviewed and approved, but shouldn't be merged before all tasks mentioned in the PR's description are addressed. For now, the only blocking task is this:

  • [ ] Update the deprecation date to ~5 months after this PR is expected to get merged. Since this can only be done once all other issues are resolved and the PR gets approved, which can still potentially take a long time, we can't know what this date should be just yet. Needs do-not-merge so we don't forget to update this date later.

ItsDrike avatar Jul 17 '22 13:07 ItsDrike

I added some description about attributes in f7086e8828909e57c885ea8c734f17e8b8a55d43, but it really should be replaced by link to API documentation from Sphinx.

image

I hope this fixes the main problem of the inheritance - you can't exactly know what attributes in class, by viewing source code.

PerchunPak avatar Sep 29 '22 15:09 PerchunPak