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

Improved ComponentSerializer #3463

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ItsLaivy
Copy link

@ItsLaivy ItsLaivy commented May 4, 2023

  • Enhanced ComponentSerializer#toString methods
  • Code enhanced

Now the ComponentSerializer#toString follows the default Minecraft's component json format. Multiples BaseComponents will be now serialized as a json array.

- Fixed ComponentSerializer#toString methods
- Code enhanced

Now the ComponentSerializer#toString follows the default Minecraft's component json format. Multiples BaseComponents will be now serialized as a json array
@md-5
Copy link
Member

md-5 commented May 5, 2023

Formatting...

@ItsLaivy
Copy link
Author

ItsLaivy commented May 5, 2023

What does this exactly means?

@md-5
Copy link
Member

md-5 commented May 5, 2023

Look at the diff and build errors please

@ItsLaivy
Copy link
Author

ItsLaivy commented May 5, 2023

Hahah i'm sorry, first time using this. This new commit should fix the tests now.

@ItsLaivy
Copy link
Author

ItsLaivy commented May 5, 2023

I need to do something more? It says the merging is blocked

@Janmm14
Copy link
Contributor

Janmm14 commented May 5, 2023

Its only about commit signing - md-5 will resolve that by signing the commit himself, nothing you need to do

 - Improved component serializer. Thanks to @MrIvanPlays (SpigotMC#3463 (comment)).
@md-5
Copy link
Member

md-5 commented Jul 24, 2023

Where do you get the default as this?

Won't this break anyone (incorrectly) relying on just accessing [0]?

Re #3490
I think a lot of MC actually can't handle arrays now

@ItsLaivy
Copy link
Author

What you mean exactly?

@md-5
Copy link
Member

md-5 commented Jul 24, 2023

Why do we want arrays?

@2008Choco
Copy link
Contributor

2008Choco commented Jul 24, 2023

I don't think we do want arrays, but I think this PR is necessary for the sake of consistency between toString() and #parse(). BC already has arrays so it's not really an option to remove them, and while we want to discourage their use I do still find it appropriate to at least improve the way arrays are handled currently :)

Won't this break anyone (incorrectly) relying on just accessing [0]

Yes. Including Spigot. Though I will be moving away from this.

@ItsLaivy
Copy link
Author

Why do we want arrays?

If you mean about this:

Multiples BaseComponents will be now serialized as a json array.

I think it's better than the extras. It's a bit buggy, with the arrays we can easily edit the base components using the Gson parser, we will not need to access the extras, and the extras of the extras. I heavily recommend using this new way.

Comment on lines +64 to +66
if ( object instanceof BaseComponent )
{
return toString( (BaseComponent) object );
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants