netvl / xml-rs

An XML library in Rust
MIT License
461 stars 113 forks source link

Add pad_self_closing config option #186

Closed tomjw64 closed 4 years ago

tomjw64 commented 4 years ago

This config option sets whether to add a space before /> in self-closing tags. Example behavior: true -> <a />, false -> <a/>

This commit also adds a single test to test the false case of this config option, as it is set to true by default.

netvl commented 4 years ago

Hi @tomjw64,

Thanks for the contribution! Note that I’m not against merging it, but I’m curious — what’s the reason for this feature? For all intents and purposes these two variants are equivalent from the XML spec point of view.

tomjw64 commented 4 years ago

Hi @netvl !

I thought I would do tiny amount of work required for this for two reasons:

1) I'm using xml-rs for a project of mine and it was useful to me to be able to specify this option to avoid an excess meaningless diffs from my source document which omits the space, without using something like `sed 's| />|/>|g' on it afterwards. I could always ignore whitespace in my diffs, but that can give me false results when it comes to significant whitespace. Another option would be using a structural diffing tools, but I am not familiar with any structural diffing tools out there for xml. This seemed reasonable to make using a plain line-based diff easier.

2) Overall, I could live without this, but I assumed since it made my life just a tiny bit easier, it might help someone else too, especially since someone had left the TODO comment for it there. Maybe if I wanted to be a little cheeky, I could make a second PR opting to remove the comment instead of adding the option 😂

Happy to contribute! And no hard feelings if this is not accepted. Let me know if I can address anything!

netvl commented 4 years ago

@tomjw64 thanks! Seems reasonable enough, and the change is simple, so I think it's okay to have it :)

Could you please still add a test for the default behavior? Even though it is default, it still would make sense to have the difference in behaviors visible in tests. For clarity, I guess you can explicitly set it to true in that test case.

tomjw64 commented 4 years ago

@netvl done!

netvl commented 4 years ago

Available in 0.8.2