Skip to content

JSON5 Support #210

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 29, 2023
Merged

JSON5 Support #210

merged 3 commits into from
Apr 29, 2023

Conversation

@Hayden-J-C
Copy link
Contributor

@Hayden-J-C Hayden-J-C commented Apr 28, 2023

Added Json5FileFormat

itzg
itzg previously approved these changes Apr 28, 2023
View reviewed changes
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

Excellent, thanks!

@itzg
Copy link
Owner

itzg commented Apr 28, 2023

Hmm, test failed. Looks like I don't have it grabbing the test report on failure, so only have the line number of the failed assertion.

@Hayden-J-C
Copy link
Contributor Author

Hayden-J-C commented Apr 28, 2023

Huh, I ran gradlew clean build before opening the PR. I'll look into it

@itzg
Copy link
Owner

itzg commented Apr 29, 2023

Oh, I wonder if it's a Windows vs Linux line ending thing. I'm away from my computer, so I can't check, there might be an ignore line endings variant of assert.

@Hayden-J-C
Copy link
Contributor Author

Hayden-J-C commented Apr 29, 2023

Got a chance to look into it, I think it's the parser acts differently on windows vs linux. Windows inserts a new line character with '\r\n' while linux just uses '\n'. Gonna try updating the "expected-setInJson5.json5" file to use '\n' instead

itzg
itzg approved these changes Apr 29, 2023
View reviewed changes
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

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

...and it still passes on Windows 😀

@itzg itzg merged commit 3b7e276 into itzg:master Apr 29, 2023
@itzg itzg mentioned this pull request Apr 29, 2023
Merged
@itzg
Copy link
Owner

itzg commented Apr 29, 2023

Included in this image build https://github.com/itzg/docker-minecraft-server/actions/runs/4836679121

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

Reviewers

@itzg itzg itzg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Hayden-J-C @itzg