Open RedPhoenixQ opened 1 month ago
Don't worry about the coverage job failing btw, it's a codecov problem, not a problem with your change.
Looks like the features
CI job failure is legitimate though.
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 yourio::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 justio::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 intoio::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?
0.37 is out already it seems! :tada: 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::Error
s?
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 91.37%. Comparing base (
a828b7e
) to head (d717b6b
). Report is 3 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
(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
...)
(that's what I get for bouncing through a bunch of issues in a short amount of time :sweat_smile: )
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 :+1:
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!
This addresses #325.
The
quick_xml::Writer
used in inferno can only ever returnstd::io::Error
but wrapped inquick_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 theirWriter
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.