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

ComponentSerializer services #343

Merged
merged 1 commit into from May 31, 2021
Merged

ComponentSerializer services #343

merged 1 commit into from May 31, 2021

Conversation

kashike
Copy link
Member

@kashike kashike commented Apr 26, 2021

No description provided.

Copy link

@octylFractal octylFractal left a comment

Choose a reason for hiding this comment

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

unkewl

@kashike kashike force-pushed the feature/component-services branch 2 times, most recently from e14baae to 00d458c Compare May 13, 2021 11:49
Copy link

@A248 A248 left a comment

Choose a reason for hiding this comment

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

As I understand, this could change the way certain component serializers are configured, possibly in a way that is not reversible by the API user.

I assume this isn't considered a breaking change for the static helper methods - GsonComponentSerializer.gson(), colorDownsamplingGson(), LegacyComponentSerializer.legacySection(), legacyAmpersand(), and the like - because the exact configuration of those serializers is not specified. (Though perhaps this lack of exact requirements could be documented)

For LegacyComponentSerializer.builder(), however, the component service has the ability to touch the builder before anything happens to it. This could mean that certain options on the LegacyComponentSerializer could be set by the service, but not unset by the user. For example, useUnusualXRepeatedCharacterHexFormat has no method to undo it.

@@ -471,6 +488,7 @@ private void applyFullFormat() {
private ComponentFlattener flattener = ComponentFlattener.basic();

BuilderImpl() {
BUILDER.accept(this); // let service provider touch the builder before anybody else touches it
Copy link

Choose a reason for hiding this comment

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

Specifically this change could irreversibly affect behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

This only provides defaults - anything can be changed by using the builder methods like normal.

@kashike
Copy link
Member Author

kashike commented May 20, 2021

I assume this isn't considered a breaking change for the static helper methods - GsonComponentSerializer.gson(), colorDownsamplingGson(), LegacyComponentSerializer.legacySection(), legacyAmpersand(), and the like - because the exact configuration of those serializers is not specified. (Though perhaps this lack of exact requirements could be documented)

Correct - not considered a breaking change.

For LegacyComponentSerializer.builder(), however, the component service has the ability to touch the builder before anything happens to it. This could mean that certain options on the LegacyComponentSerializer could be set by the service, but not unset by the user. For example, useUnusualXRepeatedCharacterHexFormat has no method to undo it.

This is true - we could introduce a method to allow this. But as is currently, no platforms do anything except providing the default ComponentFlattener to legacy and plain-text serializers.

@kashike kashike force-pushed the feature/component-services branch from 4a984c1 to 5aec9f1 Compare May 23, 2021 08:11
Adventure automation moved this from Review in progress to Reviewer approved May 23, 2021
@kashike kashike force-pushed the feature/component-services branch from 5aec9f1 to bb6be19 Compare May 31, 2021 19:34
@kashike kashike self-assigned this May 31, 2021
@kashike kashike merged commit 33fdfda into master May 31, 2021
Adventure automation moved this from Reviewer approved to Done May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants