xml-rs
xml-rs copied to clipboard
netvl
Refactored error handling to strong typing
I've noticed that the error handling in this crate is highly relying on Strings (or Cow<'static, str>, which isn't much better). This is a problem mainly for tests, performance and reliability - error cases allocate unnecessarily, tests can only compare via the .description() message (which is unreliable, you should never cast something to a String to compare the String, if it is possible to just compare the enum directly). Meaning, you can't change the error message, without everything failing - everything is stringly typed. And lastly, there is no overview over what errors can actually occur (because a String could be anything, doesn't say anything about what type of error can occur).
During making this change I needed to remove the msg() function of the Error. std::error::Error - the reason for that is that the error.description() returns a &str, which means that I can't dynamically format messages (like I can with Display). Further, the description() method is half-depreceated (and generally, Debug and Display should be used instead of .description(), mainly due to the inflexibility in error formatting). This may break existing code, which is why this PR is version breaking.
Right now the tests are failing (because the tests are stringly typed - again, please don't do this, changing the format of an error message should not break tests, the error should be independent of its formatting). I'll fix that shortly (making the test strongly typed).
Hello @fschutt, thank you very much for your pull request!
If you take a look at the repository history, you'd see that I started xml-rs development in January 2014 (5 years ago, omg o_O). Rust at that time looked very different than now, a year and a half before its first stable release. Naturally, a lot of idioms of how to do things in Rust were either undeveloped, absent or obsolete by now. This is actually the only reason why xml-rs feels "archaic" now. If I'd started writing the library from scratch now, of course, I wouldn't write it this way.
Last year I have actually started an attempt to rewrite the library, using modern idioms (e.g. copy-on-write everywhere) and incorporating things like native support for multiple encodings. You can see it in the parser-rearchitecture branch. Unfortunately, the progress on that front has been slow, due to a multitude of personal and professional reasons, and will likely stay that way for some time.
Anyway, I'll try to take a look at your changes over the next few days. I welcome any improvements in the state of the library!
Yeah, it's just that your crate is one of the most popular crates on crates.io and has dependencies everywhere, so having good foundations (like strong error handling) to build on is pretty important for me, since I depend on this library whether I like it or not. At least for me, the strongest case for refactoring to strong types is to get an overview of what things could go wrong. With strings it's like having a type-less Exception ex or [Object object]- and not knowing what the type of the exception was, so it's a pain for refactoring.
I originally wanted to refactor more, but then I thought that it would be too much refactoring for my use-case (loading XHTML documents), so I focused on just refactoring the errors, to keep the PR and the breakage small (and also get it done in time). I derived PartialOrd and Ord everwhere, simply because it doesn't hurt having it (in case you want to use a BTreeMap instead of a HashMap). I'll notify you when I fixed the test cases.
@netvl Thank you so much for all the work you put into this crate! I don't mean to sound ungrateful - and the fact that it has so many downloads is a testament to how useful this library is to the community, but, if you don't have time to review pull requests or do any further development, it might be a good idea to add that to the README so that potential users can set their expectations appropriately.
@dralley I'm very sorry about it, and you are probably right. I guess my lack of action on this front is caused by the fact that this was my first serious Rust project, and the first I've published - so it has kind of a sentimental meaning :) I did spend some time this year and last year working on a rearchitecture of the library, so maybe not everything is lost, but I do agree that this should not drag further. I think that in the near future I'll be able to make up my mind, and finally mark this crate as one looking for a maintainer, if I won't be able to find time to finish the work in the rearchitecture branch.