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

reduce code redundancy for the encoding feature #262

Closed
pchampin opened this issue Feb 9, 2021 · 2 comments · Fixed by #399
Closed

reduce code redundancy for the encoding feature #262

pchampin opened this issue Feb 9, 2021 · 2 comments · Fixed by #399
Labels
encoding Issues related to support of various encodings of the XML documents enhancement help wanted

Comments

@pchampin
Copy link
Contributor

pchampin commented Feb 9, 2021

A lot of methods are defined twice in the code, once with encoding enabled, and once without it.
Some of these methods have different result types (direct value vs. Result), and this propagate to methods calling them (which must call the former with or without a ?, depending).
(NB: as pointed out in #180, only private method should have a change of signature, but this is orthogonal, I think)

I suggest the following change:

  • instead of returning T, private methods in the non-encoding configuration should return std::result::Result<T, std::convert::Infallible>, which after compilation is equivalent to T, but is syntactically is still a result.
  • Error should implement From<Infallible> (again, this is merely to please the compiler during parsing: the ‘from method will contain an unreachable!() statement, since Infallible has no value)
  • that way, all methods calling the private method can use ?, regardless of the presence/absence of the encoding feature. That way, they don't need two different implementation. The compiler will, however, ignore the ? when the underlying error type is Infallible (i.e. when encoding is disabled).

edited fixed the path of Infallible

@tafia
Copy link
Owner

tafia commented Feb 10, 2021

This is a breaking change but I think it makes sense yes. (std::convert::Infallible not std::format)

@pchampin
Copy link
Contributor Author

A breaking change is required also by #180, to stop encoding from being a breaking feature. Kill two birds with one stone :)

And yes, I had made a mistake in the path of Infallible. This is fixed. Thanks for spotting it.

@Mingun Mingun added enhancement help wanted encoding Issues related to support of various encodings of the XML documents labels May 21, 2022
Mingun added a commit to Mingun/quick-xml that referenced this issue Jun 19, 2022
Mingun added a commit to Mingun/quick-xml that referenced this issue Jun 19, 2022
Mingun added a commit to Mingun/quick-xml that referenced this issue Jun 20, 2022
Mingun added a commit to Mingun/quick-xml that referenced this issue Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encoding Issues related to support of various encodings of the XML documents enhancement help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants