Create client.session package #3644
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
apple502j
added
refactor
Juuxel
previously approved these changes
Aug 27, 2023
|
Looks great! Two comments:
|
|
It's like "block and entities are part of the world". The telemetry package is already huge, almost larger than sessions because much of telemetry does not immediately talk to Mojang server, just like most code of blocks/entities doesn't immediately interact with rca region files. In addition, how much of report depends on sessions? It might merit its own package as well. Reports and telemetries are users of session, and you don't group your users into your subpackage (like how report.log is used by report, not using report) |
apple502j
added
the
update-base
github-actions
bot
dismissed
Juuxel’s stale review
The base branch was changed.
|
🚀 Target branch has been updated to 23w35a |
|
🚨 Please fix merge conflicts before this can be merged |
github-actions
bot
added
outdated
liach
approved these changes
Sep 2, 2023
YanisBft
approved these changes
Sep 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Client packages are mostly well-organized, except for one thing: there is no package dedicated to authlib interactions. This was fine a few years ago, when the only class that would interact with authlib was Session. Since then, however, the role of authlib in client code has expanded. This has forced us to package them in the (already-big) util package, or in the case of abuse reports, make a package directly under client package, for one relatively small feature.
I propose a repackaging, to establish "session" package and to move all authlib-interacting code there. This should have little impact outside of mods that disable reports or telemetry; however, since there are tons of mods touching this very specific stuff, it is nevertheless impactful.