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

fix missing encoding indication in pretty mode #213

Merged

Conversation

fmigneault
Copy link
Contributor

When using pretty=True, the encoding="UTF-8" indication is missing:

Json2xml({"data": [123]}, pretty=True).to_xml()
'<?xml version="1.0" ?>\n<all>\n\t<data type="list">\n\t\t<item type="int">123</item>\n\t</data>\n</all>\n'

However, it is present when pretty=False:

Json2xml({"data": [123]}, pretty=False).to_xml()
b'<?xml version="1.0" encoding="UTF-8" ?><all><data type="list"><item type="int">123</item></data></all>'

Since in pretty=True mode, the encoding remains UTF-8, the contents should still indicate it.
The proposed fix produces the following:

Json2xml({"data": [123]}, pretty=True).to_xml() 
'<?xml version="1.0" encoding="UTF-8"?>\n<all>\n\t<data type="list">\n\t\t<item type="int">123</item>\n\t</data>\n</all>\n'

Diff emphasis before/after with rendered whitespaces:

- <?xml version="1.0" ?>
+ <?xml version="1.0" encoding="UTF-8"?>
<all>
	<data type="list">
		<item type="int">123</item>
	</data>
</all>

@vinitkumar
Copy link
Owner

Thanks for your contribution @fmigneault. Could you also add tests accompanying this change, so that there are no future regressions with this?

@vinitkumar
Copy link
Owner

Actually, I will just add the test myself. Thank you so much for catching this. 🙇🏼

@vinitkumar vinitkumar merged commit 2920d71 into vinitkumar:master Aug 31, 2024
21 checks passed
vinitkumar pushed a commit that referenced this pull request Aug 31, 2024
Pretty printing didn't have encoding information in it. This test
ensures that the latest fix done in #213 has tests to verify the fix and there is no regression in the future.
@vinitkumar
Copy link
Owner

@fmigneault Just made a new release with your fix. Feel free to upgrade and let me know if all works fine?

@fmigneault
Copy link
Contributor Author

@vinitkumar
Thanks for the quick integration.
I will test it out in the following days.

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