Skip to content

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
19 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