Return `std::io::Error` from Writer methods #810

RedPhoenixQ · 2024-09-28T21:41:51Z

I recently made a PR to update this crate in another crate (inferno) and needed to remove this crates errors from the public API. While doing this I realized that the Writer could never return any error other than Error::Io. Wrapping this error in Arc and putting it in the Error enum feels unnecessary.

This does change the public API of Writer and introduces the DeError::Io kind which may not be desirable. In case this change will not happen a note in the documentation of Writer about not returning errors other than Error::Io may be useful (but may be hard to enforce, leading to outdated/incorrect docs).

I'd be more than willing to make any other changes if needed.

The other error types in `crate::errors::Error` are not applicable to the Writer. Only io error can be given and putting the io error in an `std::sync::Arc` and making users match on all other kinds is not needed. The returned `std::io::Error` can still be turned into `crate::errors::Error` if needed, but on demand.

This handles Writer now returning `std::io::Error`

Mingun · 2024-09-28T21:53:57Z

I think, it would be better to introduce a new error for serializer and close #227. Having variant in DeError just for writer looks completely disheartening to me.

Also, please add a changelog entry (do not forgot to add explicit link to section with links).

codecov-commenter · 2024-09-28T21:56:44Z

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

Codecov Report

Attention: Patch coverage is 76.19048% with 30 lines in your changes missing coverage. Please review.

Project coverage is 60.08%. Comparing base (3ebe221) to head (39b5905).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
src/errors.rs	0.00%	23 Missing ⚠️
src/se/element.rs	75.00%	3 Missing ⚠️
src/writer.rs	75.00%	3 Missing ⚠️
src/se/mod.rs	96.55%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
- Coverage   60.17%   60.08%   -0.09%     
==========================================
  Files          41       41              
  Lines       15958    15975      +17     
==========================================
- Hits         9603     9599       -4     
- Misses       6355     6376      +21

Flag	Coverage Δ
unittests	`60.08% <76.19%> (-0.09%)`	⬇️

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.

This is needed for a larger change of error return values in Writer Refs: tafia#227

RedPhoenixQ · 2024-09-29T10:02:45Z

I think, it would be better to introduce a new error for serializer and close #227. Having variant in DeError just for writer looks completely disheartening to me.

I agree and did think of making such a big change. Have added changes to split out SeError from DeError. While doing this I also see an opportunity to remove errors::Error from DeError aswell by also spitting out a DecodeError (from encoding::Decoder impl) and ReadError (from reader::read_event_impl) that could be included separately. This would make it so that no error type has more variants than needed. @Mingun Do you think this is too much for this one PR or wanted at all?

Also, please add a changelog entry (do not forgot to add explicit link to section with links).

I will be sure to add this when the scope of these changes are finalized

Mingun

Great work. Only several minor style corrections are preferred and a changelog entry.

Mingun · 2024-09-29T12:01:15Z

src/errors.rs

+                SeError::Fmt(e) => write!(f, "Formatting error: {}", e),
+                SeError::Unsupported(s) => write!(f, "Unsupported value: {}", s),
+                SeError::NonEncodable(e) => write!(f, "Malformed UTF-8: {}", e),


For better composition of different types of errors, it is better to start the error texts with a small letter. In general, we need to correct the texts for other errors too

Suggested change

SeError::Fmt(e) => write!(f, "Formatting error: {}", e),

SeError::Unsupported(s) => write!(f, "Unsupported value: {}", s),

SeError::NonEncodable(e) => write!(f, "Malformed UTF-8: {}", e),

SeError::Fmt(e) => write!(f, "formatting error: {}", e),

SeError::Unsupported(s) => write!(f, "unsupported value: {}", s),

SeError::NonEncodable(e) => write!(f, "malformed UTF-8: {}", e),

Mingun · 2024-09-29T12:07:39Z

src/se/key.rs

@@ -1,4 +1,4 @@
-use crate::errors::serialize::DeError;
+use crate::errors::serialize::SeError;


In all other places, you used this path to error. Let's here too, for consistency:

Suggested change

use crate::errors::serialize::SeError;

use crate::se::SeError;

Mingun · 2024-09-29T12:11:01Z

tests/serde-se.rs

@@ -1,7 +1,7 @@
 use quick_xml::de::from_str;
 use quick_xml::se::Serializer;
 use quick_xml::utils::Bytes;
-use quick_xml::DeError;
+use quick_xml::SeError;


For consistency, this also could be imported as quick_xml::se::SeError in line 2

Mingun · 2024-09-29T12:11:35Z

src/writer.rs

@@ -280,8 +278,8 @@ impl<W: Write> Writer<W> {
    /// # use serde::Serialize;
    /// # use quick_xml::events::{BytesStart, Event};
    /// # use quick_xml::writer::Writer;
-    /// # use quick_xml::DeError;
-    /// # fn main() -> Result<(), DeError> {
+    /// # use quick_xml::SeError;


For consistency

Suggested change

/// # use quick_xml::SeError;

/// # use quick_xml::se::SeError;

RedPhoenixQ · 2024-09-29T12:37:59Z

I have worked on some changes to narrow other return values, like from the encoding module. I'll submit a separate PR after this has landed with these changes if that would be of interest

Mingun · 2024-09-29T14:25:11Z

Unrelated changes in changelog.md is unwanted. Could you revert them? Especially two blank lines used to separate releases.

I have worked on some changes to narrow other return values, like from the encoding module. I'll submit a separate PR after this has landed with these changes if that would be of interest

Any PR is welcome

RedPhoenixQ · 2024-09-29T15:20:36Z

Unrelated changes in changelog.md is unwanted. Could you revert them? Especially two blank lines used to separate releases.

My bad, didn't realize some formatter had kicked in. Fixed

Mingun · 2024-09-29T17:03:56Z

Thanks!

Mingun · 2024-09-29T17:26:13Z

@RedPhoenixQ, for the reference: I will wait for your other PR for a some time (to the end of next week) before release a new version.

RedPhoenixQ added 2 commits September 28, 2024 23:17

Add ioError to DeError

1afe6d0

This handles Writer now returning `std::io::Error`

RedPhoenixQ added 2 commits September 29, 2024 11:51

Split SeError from DeError

f0e17be

This is needed for a larger change of error return values in Writer Refs: tafia#227

Remove unused DeError kinds and impls

41c57d8

Mingun approved these changes Sep 29, 2024

View reviewed changes

Minor corrections and consistency

21ec992

RedPhoenixQ force-pushed the io-errors branch from 059d4a0 to 22d8fca Compare September 29, 2024 15:19

Mingun linked an issue Sep 29, 2024 that may be closed by this pull request

quick_xml::se::to_string returns a DeError #227

Closed

Add changelog entry

39b5905

Mingun force-pushed the io-errors branch from 22d8fca to 39b5905 Compare September 29, 2024 17:03

Mingun merged commit e93181c into tafia:master Sep 29, 2024
7 checks passed

RedPhoenixQ mentioned this pull request Sep 29, 2024

Make Errors more "narrow" #811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return `std::io::Error` from Writer methods #810

Return `std::io::Error` from Writer methods #810

RedPhoenixQ commented Sep 28, 2024

Mingun commented Sep 28, 2024 •

edited

Loading

codecov-commenter commented Sep 28, 2024 •

edited

Loading

RedPhoenixQ commented Sep 29, 2024

Mingun left a comment

Mingun Sep 29, 2024

Mingun Sep 29, 2024

Mingun Sep 29, 2024

Mingun Sep 29, 2024

RedPhoenixQ commented Sep 29, 2024 •

edited

Loading

Mingun commented Sep 29, 2024

RedPhoenixQ commented Sep 29, 2024

Mingun commented Sep 29, 2024

Mingun commented Sep 29, 2024

		@@ -1,4 +1,4 @@
		use crate::errors::serialize::DeError;
		use crate::errors::serialize::SeError;

	use crate::errors::serialize::SeError;
	use crate::se::SeError;

	/// # use quick_xml::SeError;
	/// # use quick_xml::se::SeError;

Return std::io::Error from Writer methods #810

Return std::io::Error from Writer methods #810

Conversation

RedPhoenixQ commented Sep 28, 2024

Mingun commented Sep 28, 2024 • edited Loading

codecov-commenter commented Sep 28, 2024 • edited Loading

Codecov Report

RedPhoenixQ commented Sep 29, 2024

Mingun left a comment

Choose a reason for hiding this comment

Mingun Sep 29, 2024

Choose a reason for hiding this comment

Mingun Sep 29, 2024

Choose a reason for hiding this comment

Mingun Sep 29, 2024

Choose a reason for hiding this comment

Mingun Sep 29, 2024

Choose a reason for hiding this comment

RedPhoenixQ commented Sep 29, 2024 • edited Loading

Mingun commented Sep 29, 2024

RedPhoenixQ commented Sep 29, 2024

Mingun commented Sep 29, 2024

Mingun commented Sep 29, 2024

Return `std::io::Error` from Writer methods #810

Return `std::io::Error` from Writer methods #810

Mingun commented Sep 28, 2024 •

edited

Loading

codecov-commenter commented Sep 28, 2024 •

edited

Loading

RedPhoenixQ commented Sep 29, 2024 •

edited

Loading