Make Errors more "narrow" #811

RedPhoenixQ · 2024-09-29T19:15:06Z

This was discussed in #810 here.

This splits up error types so that there is almost one type for each module, which narrows the amount of error variants that is returned from each function.

For the functions where two or more error types may be returned the new combined_error! macro is used to create a new error enum that holds these variants with most error impl's automatically done.

The old errors::Error is still present and public. All other new errors implement From<_> for errors::Error so that the provided errors::Result type can still be used to try (?) any function from this crate.

I haven't spent much time adding/editing docs for these changes since most of them still apply without changes. Maybe the docs for errors::Error should be changed to make it clear that it will not be given out as an error from anywhere directly. There are also some names of the new error types that might need changing

codecov-commenter · 2024-09-29T19:40:18Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 48.20513% with 101 lines in your changes missing coverage. Please review.

Project coverage is 60.14%. Comparing base (39b5905) to head (00a0962).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/errors.rs	4.25%	45 Missing ⚠️
src/name.rs	69.13%	25 Missing ⚠️
src/encoding.rs	47.05%	18 Missing ⚠️
src/events/mod.rs	57.14%	6 Missing ⚠️
examples/read_nodes.rs	0.00%	4 Missing ⚠️
src/escape.rs	0.00%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #811      +/-   ##
==========================================
+ Coverage   60.08%   60.14%   +0.05%     
==========================================
  Files          41       41              
  Lines       15975    15985      +10     
==========================================
+ Hits         9599     9614      +15     
+ Misses       6376     6371       -5

Flag	Coverage Δ
unittests	`60.14% <48.20%> (+0.05%)`	⬆️

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

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

RedPhoenixQ · 2024-09-29T20:22:24Z

Sorry for the force pushes, it keeps running my usage example as a doc test and the annotations (like '''rust) mean different things in different rust version apparently

Mingun

Some doc links became broken, that need to be fixed

> cargo doc --all-features
warning: unresolved link to `encoding`
  --> src\encoding.rs:22:18
   |
22 | /// If feature [`encoding`] is disabled, the EncodingError is always `DecodeError::Utf8`:
   |                  ^^^^^^^^ no item named `encoding` in scope
   |
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
   = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

warning: unresolved link to `Error::IllFormed`
   --> src\reader\buffered_reader.rs:308:75
    |
308 |     /// If a corresponding [`End`] event is not found, an error of type [`Error::IllFormed`]
    |                                                                           ^^^^^^^^^^^^^^^^ no item named `Error` in scope

warning: unresolved link to `Error::IllFormed`
  --> src\reader\slice_reader.rs:88:75
   |
88 |     /// If a corresponding [`End`] event is not found, an error of type [`Error::IllFormed`]
   |                                                                           ^^^^^^^^^^^^^^^^ no item named `Error` in scope

warning: `quick-xml` (lib doc) generated 3 warnings

I'm fine with the first 3 commits, but I'm unsure about the 4th (other two commits just the follow-ups fixing the compilation errors). I'm not sure that such a fine breakdown of errors will be convenient for work. In most cases, you will not be interested in a specific type of error and it will still be thrown higher, but the presence of many types of errors will complicate such throwing (it's not for nothing that things like anyhow::Error appeared).

Do you have opposite experience?

You can fix things and make a force push, GitHub UI provides a way to compare between force pushes, so it is fine). I would prefer to have a history without fix-up commits, so it would be great if each commit:

in a compilable state on CI (which means it is compiled fine with all CI tested combinations of flags, in practice usually check of cargo test and cargo test --all-features is enough)
cargo doc --all-features does not report warnings
each commit updates changelog with the relevant changes (so it is easely to understand later in which commit that change was made)

Mingun · 2024-10-01T08:29:38Z

src/errors.rs

-impl From<Utf8Error> for Error {
-    /// Creates a new `Error::NonDecodable` from the given error
+impl From<EncodingError> for Error {
+    /// Creates a new `Error::DecodeError` from the given error


You named this variant EncodingError

Suggested change

/// Creates a new `Error::DecodeError` from the given error

/// Creates a new [`Error::EncodingError`] from the given error

Or do the opposite: name the variant DecodeError. Maybe this is preferred, because this error is possible only when reading

Mingun · 2024-10-01T08:33:33Z

src/encoding.rs

+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match self {
+            Self::Utf8(e) => Some(e),
+            #[allow(unreachable_patterns)]


Why not

Suggested change

#[allow(unreachable_patterns)]

#[cfg(feature = "encoding")]

?

Mingun · 2024-10-01T08:37:46Z

src/encoding.rs

+///
+/// If feature [`encoding`] is disabled, the EncodingError is always `DecodeError::Utf8`:
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum EncodingError {


Need to add #[non_exhaustive] so the consumers forced to explicitly handle wildcard variant. Otherwise if some other crate in the dependency tree activates the encoding feature, the crates without wildcard handling and without encoding feature will fail to compile.

Suggested change

pub enum EncodingError {

#[non_exhaustive]

pub enum EncodingError {

Mingun · 2024-10-01T08:40:48Z

src/encoding.rs

+            Self::Utf8(e) => write!(f, "UTF-8 error: {}", e),
+            #[cfg(feature = "encoding")]
+            Self::Other(encoding) => write!(f, "Error occured when decoding {}", encoding.name()),


Make texts start with lower-case letter and unify them. I assume that error is used only when decoding, otherwise it is needed to tweak messages

Suggested change

Self::Utf8(e) => write!(f, "UTF-8 error: {}", e),

#[cfg(feature = "encoding")]

Self::Other(encoding) => write!(f, "Error occured when decoding {}", encoding.name()),

Self::Utf8(e) => write!(f, "cannot decode input using UTF-8: {}", e),

#[cfg(feature = "encoding")]

Self::Other(encoding) => write!(f, "cannot decode input using {}", encoding.name()),

Mingun · 2024-10-01T08:44:06Z

src/encoding.rs

@@ -1,14 +1,10 @@
 //! A module for wrappers that encode / decode data.

-use std::borrow::Cow;
+use std::{borrow::Cow, str::Utf8Error};


I prefer to not have nested imports:

Suggested change

use std::{borrow::Cow, str::Utf8Error};

use std::borrow::Cow;

use std::str::Utf8Error;

Mingun · 2024-10-01T08:59:16Z

src/name.rs

+    /// Error for when a reserved namespace is set incorrectly.
+    ///
+    /// This error returned in following cases:
+    /// - the XML document attempts to bind `xml` prefix to something other than
+    ///   `http://www.w3.org/XML/1998/namespace`
+    /// - the XML document attempts to bind `xmlns` prefix
+    /// - the XML document attempts to bind some prefix (except `xml`) to
+    ///   `http://www.w3.org/XML/1998/namespace`
+    /// - the XML document attempts to bind some prefix to
+    ///   `http://www.w3.org/2000/xmlns/`
+    InvalidPrefixBind {


If we split error into small parts, maybe make a dedicated variant for each listed variant?

If we split error into small parts, maybe make a dedicated variant for each listed variant?

I will add a commit for this soon. Need to understand the code and the linked standard to see which error applies were which may take a bit longer.

Done in commit 8002cc6

Mingun · 2024-10-01T09:03:48Z

src/name.rs

+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            Self::UnknownPrefix(prefix) => {
+                f.write_str("Unknown namespace prefix '")?;


As already mentioned, if we bring order in errors, make all texts with a small letter:

Suggested change

f.write_str("Unknown namespace prefix '")?;

f.write_str("unknown namespace prefix '")?;

Mingun · 2024-10-01T09:04:00Z

src/name.rs

+                f.write_str("'")
+            }
+            Self::InvalidPrefixBind { prefix, namespace } => {
+                f.write_str("The namespace prefix '")?;


Same here

Suggested change

f.write_str("The namespace prefix '")?;

f.write_str("the namespace prefix '")?;

Mingun · 2024-10-01T09:36:04Z

src/errors.rs

+        $($variant:ident($error:path $(, $inner_type:path)?) $fmt_str:literal),+ $(,)?
+    ) => {
+        #[derive(Debug)]
+        #[allow(missing_docs)]


I would like to update macro and write a documentation for each generated variant under which circumstances it will be returned. That is not always obvious from the name and description of the inner error.

Suggested change

#[allow(missing_docs)]

Also, I would prefer to have more traditional syntax for defining enums, in order to in the end the written code will look like:

combined_error! { /// Doc // derives pub enum SpecificError { /// Variant 1 doc Variant1(Variant1Error) => "display 1 text", /// Variant 2 doc Variant2(Variant2Error) => "display 2 text", } }

Please also derive Debug for each error type.

RedPhoenixQ · 2024-10-01T10:32:01Z

Some doc links became broken, that need to be fixed

Compiling the docs was an oversight. Will fix.

I'm fine with the first 3 commits, but I'm unsure about the 4th (other two commits just the follow-ups fixing the compilation errors). I'm not sure that such a fine breakdown of errors will be convenient for work. In most cases, you will not be interested in a specific type of error and it will still be thrown higher, but the presence of many types of errors will complicate such throwing (it's not for nothing that things like anyhow::Error appeared).

Do you have opposite experience?

I'm mostly interested in not having irrelevant options when calling lower level apis. I will agree that the attributes methods are inconvenient when returning ReadError (like the read_node example). I still think that having these methods return AttrError is better for when only using those apis.

I'm torn on whether this is better, especially when it's common for ReadError and AttrError appear in the same function. I still prefer returning AttrError but I agree it may not be worth it.

If this split beyond EncodingError and NamespaceError is not desired, the combined_error! macro could also be removed entirely.

You can fix things and make a force push, GitHub UI provides a way to compare between force pushes, so it is fine). I would prefer to have a history without fix-up commits, so it would be great if each commit:

in a compilable state on CI (which means it is compiled fine with all CI tested combinations of flags, in practice usually check of cargo test and cargo test --all-features is enough)

cargo doc --all-features does not report warnings

each commit updates changelog with the relevant changes (so it is easely to understand later in which commit that change was made)

Absolutely. The docs was an oversight and I know that one commit doesn't pass the tests. I will go back and rework all of these commits.

RedPhoenixQ · 2024-10-01T10:34:52Z

When the errors are being changed anyway, there's an opportunity to be consistent about whether Error enum variants should be named SomeError::Io or SomeError::IoError. Any preference here?

Mingun · 2024-10-01T11:00:17Z

I still think that having these methods return AttrError is better for when only using those apis.

When we declare in API a more wide error that is really could happen, I'm fine to narrow the result type, but introducing new fine-granulated error types for that, I think, would be overkill.

The problem is that the API may not be well-established, and with the introduction of validation checks, it may turn out that some functions will return more errors. Some Linux package maintainers already complained, that quick-xml API changes too quickly :).

If this split beyond EncodingError and NamespaceError is not desired, the combined_error! macro could also be removed entirely.

Yes. So for now please left only the changes from the first 3 commits. Maybe in time the 4th commit also would be welcomed, who knows :)?

Any preference here?

SomeError::Io

RedPhoenixQ · 2024-10-01T19:41:42Z

I belive I have fixed all issues from the previous review. The changelog is now incrementally updated every commit and all commits pass cargo test and cargo doc (with --all-features).

I have still included the AttrError change as I didn't understand if it should be discarded or not. Will remove it if you want.

… deserialization error messages More clear and may slightly increase compile time

…e of type Make code more consistent

This mostly allows for decode functions to return a smaller more accurate error

…mplements PartialEq

Mingun

I polished the PR slightly:

Normalized all error messages to start from lowercase
Made error-related code more consistent: removed unnecessary usages of write! macro, used Self instead of type name where possible (that two changes was related to the errors so it was logical to made them in that PR)
Implemented PartialEq and Eq for NamespaceError to simplify testing of the methods returning results with that error
That allowed to change tests from manual matching on result to use assert_eq! which will provide nice diff if failed
You forgot to return Some from Error::source for Error::Namespace -- fixed
Used tuple form instead of struct form for NamespaceError errors. Usually struct form with one field is not used
Removed BangType from changelog. It is internal type not visible to users

Mingun · 2024-10-12T14:57:15Z

Thanks!

RedPhoenixQ force-pushed the error-narrowing branch 2 times, most recently from a94ee2a to 004442c Compare September 29, 2024 19:31

RedPhoenixQ force-pushed the error-narrowing branch 2 times, most recently from 8fcf6fd to a0ab37f Compare September 29, 2024 20:13

Mingun requested changes Oct 1, 2024

View reviewed changes

RedPhoenixQ force-pushed the error-narrowing branch from a0ab37f to 4bbb94b Compare October 1, 2024 19:33

RedPhoenixQ requested a review from Mingun October 4, 2024 18:14

Mingun and others added 10 commits October 12, 2024 18:13

Make all error test started from lower-case letter

d112667

Do not use write! macro when not required and slightly improve number…

82cfe51

… deserialization error messages More clear and may slightly increase compile time

Use Self in match expressions and from implementations instead of nam…

13d14ce

…e of type Make code more consistent

Split NamespaceError from the Error type

6dbd39a

Split EncodingError from the Error type

a975a82

This mostly allows for decode functions to return a smaller more accurate error

Rename EscapeError variant to match others

e36d743

Return SyntaxError from BangType

38e11c7

Return AttrError from attribute methods

d35e497

Split reserved namespace binding errors

8a3a140

Use assert_eq! instead of manual matches because now NamespaceError i…

00a0962

…mplements PartialEq

Mingun force-pushed the error-narrowing branch from 8002cc6 to 00a0962 Compare October 12, 2024 14:44

Mingun approved these changes Oct 12, 2024

View reviewed changes

Mingun merged commit 6eea6bb into tafia:master Oct 12, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Errors more "narrow" #811

Make Errors more "narrow" #811

RedPhoenixQ commented Sep 29, 2024

codecov-commenter commented Sep 29, 2024 •

edited

Loading

RedPhoenixQ commented Sep 29, 2024

Mingun left a comment

Mingun Oct 1, 2024

Mingun Oct 1, 2024

Mingun Oct 1, 2024

Mingun Oct 1, 2024

Mingun Oct 1, 2024

Mingun Oct 1, 2024

RedPhoenixQ Oct 1, 2024

RedPhoenixQ Oct 1, 2024

Mingun Oct 1, 2024

Mingun Oct 1, 2024

Mingun Oct 1, 2024

RedPhoenixQ commented Oct 1, 2024

RedPhoenixQ commented Oct 1, 2024

Mingun commented Oct 1, 2024

RedPhoenixQ commented Oct 1, 2024

Mingun left a comment

Mingun commented Oct 12, 2024

	/// Creates a new `Error::DecodeError` from the given error
	/// Creates a new [`Error::EncodingError`] from the given error

	pub enum EncodingError {
	#[non_exhaustive]
	pub enum EncodingError {

	use std::{borrow::Cow, str::Utf8Error};
	use std::borrow::Cow;
	use std::str::Utf8Error;

	f.write_str("Unknown namespace prefix '")?;
	f.write_str("unknown namespace prefix '")?;

	f.write_str("The namespace prefix '")?;
	f.write_str("the namespace prefix '")?;

Make Errors more "narrow" #811

Make Errors more "narrow" #811

Conversation

RedPhoenixQ commented Sep 29, 2024

codecov-commenter commented Sep 29, 2024 • edited Loading

Codecov Report

RedPhoenixQ commented Sep 29, 2024

Mingun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedPhoenixQ commented Oct 1, 2024

RedPhoenixQ commented Oct 1, 2024

Mingun commented Oct 1, 2024

RedPhoenixQ commented Oct 1, 2024

Mingun left a comment

Choose a reason for hiding this comment

Mingun commented Oct 12, 2024

codecov-commenter commented Sep 29, 2024 •

edited

Loading