Bump quick-xml to 0.37.0 and remove it from public APIs

[RedPhoenixQ]

This addresses #325.

The quick_xml::Writer used in inferno can only ever return std::io::Error but wrapped in quick_xml::Error. To fix this a small helper function has been added to "unwrap" this io error and return that as the public API.

I'm going to submit a PR to quick-xml to only give out std::io::Error from their Writer to make this behavior clear. There might be a good reason that I don't see.

I'm happy to make any other changes, like making a custom Error type if that's nicer for the public API.

[jonhoo]

This is great, thank you, and thanks for also filing a change to quick-xml! I wonder whether we should wait for 0.37 for when your io::Error-only change is present? I suppose with the type now gone from the public API, we should be able to transparently upgrade later anyway, so there's no huge reason to delay.

One question I have is whether we should be exposing io::Error directly too, or whether we should introduce an opaque error type (say, inferno::Error) that internally (for now) is just io::Error), but that we could extend in the future if needed to contain other kinds of errors if we pick up dependencies that can't as easily be turned into io::Error 🤔 What do you think?

Also, would you mind updating CHANGELOG.md?

[jonhoo]

Don't worry about the coverage job failing btw, it's a codecov problem, not a problem with your change.

[jonhoo]

Looks like the features CI job failure is legitimate though.

[RedPhoenixQ]

This is great, thank you, and thanks for also filing a change to quick-xml! I wonder whether we should wait for 0.37 for when your io::Error-only change is present? I suppose with the type now gone from the public API, we should be able to transparently upgrade later anyway, so there's no huge reason to delay.

It should be fine as the PR in quick_xml has already merged and it unlikely that it would change again before a new release.

One question I have is whether we should be exposing io::Error directly too, or whether we should introduce an opaque error type (say, inferno::Error) that internally (for now) is just io::Error), but that we could extend in the future if needed to contain other kinds of errors if we pick up dependencies that can't as easily be turned into io::Error 🤔 What do you think?

If we do, would that type have #[non_exhaustive] to prevent breaking changes in the case where more errors are added? If not it would be a breaking change either way. I don't know what the best shape of the custom error would be. io::Error isn't Clone so the custom error can't be without Arc, which is unfortunate. If that case comes io::Error does have an ErrorKind::Other that would allow for passing any error as io::Error, but not as nice or explicit. This would at least temporarily allow for the new error without a breaking change and maybe that enough.

I'm willing to create a custom error if that's nicer. I would probably do it without Clone och PartialEq as io::Error doesn't implement it and thus neither do we, or should we in that case?

[jonhoo]

0.37 is out already it seems! 🎉
https://github.com/tafia/quick-xml/releases/tag/v0.37.0

For an opaque error, I was thinking it would be a struct, not an enum, and indeed with #[non_exhaustive]. No public fields (or variants) so that we can change what's inside the error at will. Sort of like reqwest::Error for example. And yeah, completely agree it probably shouldn't be Clone or PartialEq.

What we're balancing here is how likely it is that we'd ever want custom errors in inferno, possibly from dependencies, that we don't want to coerce into io::Error. Going to seek some input here from @djc who maintains cargo-flamegraph, as they are one of the biggest consumer of fantoccini — how much of a pain would it be if we introduced our own error variant instead of just producing io::Errors?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.37%. Comparing base (a828b7e) to head (d717b6b).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
- Coverage   91.38%   91.37%   -0.02%     
==========================================
  Files          20       20              
  Lines        4483     4440      -43     
==========================================
- Hits         4097     4057      -40     
+ Misses        386      383       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RedPhoenixQ]

0.37 is out already it seems! 🎉 https://github.com/tafia/quick-xml/releases/tag/v0.37.0

Must have happened today! I updated the title and bumped to 0.37.

I'm still okay with creating a custom error if a decision gets made on it.

[djc]

(flamegraph has an inferno dependency, not fantoccini -- I think you got some crates confused? 😄)

flamegraph immediately converts any inferno errors into anyhow::Error, so it really doesn't care about anything other than that the std::error::Error trait is implemented.

(Note, I have been working for a few years on instant-xml as an alternative to quick-xml. Might be interesting for inferno, although it doesn't support everything inferno needs -- so far noticed that there's no support for pretty printing and its Serializer interface wraps fmt::Write rather than io::Write. Feels weird that File/BufWriter don't implement fmt::Write...)

[jonhoo]

(that's what I get for bouncing through a bunch of issues in a short amount of time 😅 )

Okay, that sounds good. In that case, let's just stick with io::Error like you have it now @RedPhoenixQ!

On instant-xml, that does sound interesting. We don't particularly care about the pretty printing I don't think 👍

Otherwise, just a heads up that I'll be on holiday for the next three weeks, so won't have a chance to land this until I get back :) But I'll approve the PR to make it clear that the intent is to land it + release as a new major version when I return!