Skip to content
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

Merged
merged 24 commits into from
Nov 3, 2023
Merged

Rework errors #675

merged 24 commits into from
Nov 3, 2023

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Oct 28, 2023

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.

Strange, that cargo_fmt did reformat the code that previously
was acceptable and didn't changed in the previous commit
…r `visit_string` instead

It is responsibility of a `Visitor` to return an error if the provided data can not be handled
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2023

Codecov Report

Merging #675 (ab4029f) into master (120e074) will decrease coverage by 0.26%.
The diff coverage is 57.98%.

❗ 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     
Flag Coverage Δ
unittests 65.06% <57.98%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/lib.rs 20.08% <100.00%> (ø)
src/reader/buffered_reader.rs 85.51% <ø> (ø)
src/reader/ns_reader.rs 59.00% <ø> (ø)
src/reader/slice_reader.rs 100.00% <100.00%> (ø)
src/reader/state.rs 98.73% <100.00%> (+0.01%) ⬆️
src/de/key.rs 94.41% <80.00%> (-0.76%) ⬇️
src/de/map.rs 90.69% <88.88%> (+0.82%) ⬆️
src/de/var.rs 90.47% <0.00%> (ø)
src/se/key.rs 96.70% <0.00%> (ø)
src/de/simple_type.rs 93.59% <95.65%> (-0.30%) ⬇️
... and 9 more

Mingun and others added 4 commits October 29, 2023 03:40
…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
src/errors.rs Outdated Show resolved Hide resolved
@@ -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`]
Copy link
Collaborator

@dralley dralley Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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`]

Copy link
Collaborator Author

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.

Copy link
Collaborator

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]
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

src/errors.rs Outdated Show resolved Hide resolved
src/errors.rs Show resolved Hide resolved
Copy link
Collaborator

@dralley dralley left a 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

@Mingun Mingun merged commit b95b503 into tafia:master Nov 3, 2023
6 checks passed
@Mingun Mingun deleted the errors branch November 3, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants