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

Added SetJSONIndent to Encoder #57

Merged
merged 3 commits into from
Jul 11, 2022
Merged

Added SetJSONIndent to Encoder #57

merged 3 commits into from
Jul 11, 2022

Conversation

xackery
Copy link
Contributor

@xackery xackery commented Jul 9, 2022

This pretty prints encoded gltf, e.g.

{"buffers":[{"uri":"data:application/octet-stream;base64

turns to

{
	"buffers": [
		{
			"uri": "data:application/octet-stream;base64

just by setting e.g.

enc := gltf.NewEncoder(w)
enc.AsBinary = false
enc.SetIndent("", "\t") // <-- this line
err = enc.Encode(doc)

Copy link
Owner

@qmuntal qmuntal left a comment

Choose a reason for hiding this comment

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

Good addition, thanks!

encode.go Outdated Show resolved Hide resolved
@xackery xackery changed the title Added SetIndent to Encoder Added SetJSONIndent to Encoder Jul 10, 2022
@qmuntal
Copy link
Owner

qmuntal commented Jul 10, 2022

Can you add a test which roundtrips (encode-> decode -> compare) an indented gltf just to be sure that it does not break anything?

@xackery
Copy link
Contributor Author

xackery commented Jul 11, 2022

Can you add a test which roundtrips (encode-> decode -> compare) an indented gltf just to be sure that it does not break anything?

I added additional tests inside encode_test that at least cover the new code, I am unsure if you want even more testing? From what I can tell, encode testing is exclusive to encode_test, but I basically do encode/decode/compare with this new commit ... Let me know if this satisfies your ask

Copy link
Owner

@qmuntal qmuntal left a comment

Choose a reason for hiding this comment

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

LGTM!

@qmuntal qmuntal merged commit 3ea49a1 into qmuntal:master Jul 11, 2022
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.

2 participants