-
Notifications
You must be signed in to change notification settings - Fork 238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework errors #675
Rework errors #675
Conversation
… only if the deserializer has a bug
…r `visit_string` instead It is responsibility of a `Visitor` to return an error if the provided data can not be handled
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #675 +/- ##
==========================================
- Coverage 65.31% 65.06% -0.26%
==========================================
Files 38 38
Lines 17837 17835 -2
==========================================
- Hits 11650 11604 -46
- Misses 6187 6231 +44
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…l `deserialize_str` instead Deserializer methods are only hints, if deserializer could not satisfy request, it should return the data that it has. It is responsibility of a Visitor to return an error if it does not understand the data UnitDeserializer::new() available only since serde 1.0.139 (serde-rs/serde#2246)
Provide to visitor a string or a map instead. Deserializer methods are only hints, if deserializer could not satisfy request, it should return the data that it has. It is responsibility of a Visitor to return an error if it does not understand the data
…e, tuple or struct from QName Provide to visitor a unit instead. Deserializer methods are only hints, if deserializer could not satisfy request, it should return the data that it has. It is responsibility of a Visitor to return an error if it does not understand the data
…:Syntax(SyntaxError)`
src/reader/buffered_reader.rs
Outdated
@@ -316,7 +316,7 @@ impl<R: BufRead> Reader<R> { | |||
/// Manages nested cases where parent and child elements have the _literally_ | |||
/// same name. | |||
/// | |||
/// If corresponding [`End`] event will not be found, the [`Error::UnexpectedEof`] | |||
/// If corresponding [`End`] event will not be found, the [`Error::IllFormed`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// If corresponding [`End`] event will not be found, the [`Error::IllFormed`] | |
/// If a corresponding [`End`] event is not found, an error of type [`Error::IllFormed`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dralley, is not "the an" is a typo that you use two articles in a row? I've never seen such construct before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooof, that's a typo in my suggestion. Apologies.
@@ -1867,6 +1867,7 @@ macro_rules! deserialize_primitives { | |||
} | |||
|
|||
/// Character represented as [strings](#method.deserialize_str). | |||
#[inline] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you actually found this to make a difference?
The primary purpose of #[inline]
is to allow inlining across compilation units. Within a compilation unit, the compiler is just going to decide on its own and mostly ignore this attribute unless you say #[inline(always)]
.
If these functions are not going to be called from any external library, I doubt it would make a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, added just for consistence. Actually I want to run some profiler one day, just need time to learn how to do that and find a which one that could be run on Windows. Most tools supports only Linux. cargo flamegraph
looks promising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a nice change
…sults of `read_to_end` Co-authored-by: Daniel Alley <[email protected]>
…`DeError::UnexpectedEof`
…n read text in a tag and cover that case by test
…cases where Eof was received while waiting end tag
… cover case for a field of a sequential type
… explicitly show this using `unreachable!` when we in that branch, `Start` or `Text` event already was picked
…no corresponding open tag
…te 2 different errors
… in cases where start tag is missing
This PR does cleanup of our errors, making them more consistent. In particular, now syntax errors decoupled from other errors and this will allow us to have
no_std
XML parser in the future.In addition, I also review the way how errors is reported by the serde deserializer. I already wrote my points about correct deserializer behavior here and in this PR I implement them -- I avoid to return errors if requested data does not match existing data. Instead deserializer will provide the data and the
Visitor
will return an error if it wish.