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

ensure numbers where we expected booleans parse correctly. #1108

Open
wants to merge 3 commits into
base: main/4
Choose a base branch
from

Conversation

456dev
Copy link

@456dev 456dev commented Sep 9, 2024

Boolean.parseBoolean expects the literal "true", which doesn't match the number 1.

The fix is to parse "1" as truthy, and all other values as falsy, like Boolean.parseBoolean() does.

fixes PaperMC/Velocity#1286

I'm assuming its meant to accept "1" as valid, seeing as it was specifically peeking for Numbers.
In velocity at least, "1" in json is emitted by the nbt component converter, from bytes

Boolean.parseBoolean expects the literal "true", which doesn't match the number 1.

The fix is to parse "1" as truthy, and all other values as falsy, like Boolean.parseBoolean() does.

fixes PaperMC/Velocity#1286
@456dev
Copy link
Author

456dev commented Sep 9, 2024

Oh, looks like it might be intentional, if there is a test for it.

does that just make this velocities problem?

@zml2008
Copy link
Member

zml2008 commented Sep 9, 2024

I think this is a format change from since component serialization started using Codecs? so it's a case of adventure's tests being outdated against the newly changed spec.

@456dev
Copy link
Author

456dev commented Sep 9, 2024

I'm still not 100% sure if this should change, as looking at the codecs etc, the vanilla jsonOps only parses / emits standard json bools, so this is more permissive than how minecraft behaves with json.
(only nbt ops parses bools using bytes i think)

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

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

Assuming this is too minor a change to bother with a serialization option here, LGTM.

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

Successfully merging this pull request may close these issues.

Failed to parse Component from Json (packet) correctly
3 participants