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

Add <event>::borrow() and some minor stuff #416

merged 7 commits into from Jul 12, 2022


Copy link

@Mingun Mingun commented Jul 10, 2022

This PR makes some refactoring that I do on my path to the implementing namespaces support.

Currently I need to fix lifetime issues and I plan split reader and writer events into separate structs, which requires some preparing work. In order to not make that PR too big I made this PR with this work.

I fixed some intradoc links, move tests to appropriate place and introduced two improvements:

  • BytesStart and BytesEnd structs now implements From<QName> that allows to create them easely (useful for writing)
  • Writer::write_event now consumes event using an Into trait. AsRef trait, used before, does not allow to accept two unrelated structs which reader::Event and writer::Event will be. On the contrary, writer::Event will implement From<reader::Event> which would allow to write reader events as before. In order to not lose the ownership, the Event::borrow() method is introduced, as a counterpart to the Event::into_owned() method, so Writer::write_event could consume only borrowed event

…c links

Compiler will check existence of the intradoc links.

Cross-references in the Deserializer couldn't be converted to intradoc links,
because trait is implemented for `&mut` and rustdoc cannot resolve that links
Copy link

dralley commented Jul 10, 2022

I plan split reader and writer events into separate structs

Why? This would be a bit unfortunate from a usability perspective, let's say you wanted to write a XML formatter and you just wanted to shuffle events from the reader directly over to a writer with a minimum amount of processing.

Copy link

Codecov Report

Merging #416 (60f7087) into master (2eecf00) will decrease coverage by 0.36%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #416      +/-   ##
- Coverage   49.60%   49.24%   -0.37%     
  Files          22       22              
  Lines       13967    13984      +17     
- Hits         6928     6886      -42     
- Misses       7039     7098      +59     
Flag Coverage Δ
unittests 49.24% <66.66%> (-0.37%) ⬇️

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

Impacted Files Coverage Δ
src/de/ 72.85% <ø> (ø)
src/ 1.55% <ø> (ø)
src/events/ 93.58% <ø> (ø)
src/events/ 69.91% <28.57%> (-9.55%) ⬇️
src/ 60.19% <50.00%> (+0.06%) ⬆️
src/ 49.28% <64.00%> (+0.27%) ⬆️
src/de/ 75.32% <100.00%> (-2.07%) ⬇️
src/de/ 92.15% <100.00%> (+0.32%) ⬆️
src/ 88.37% <100.00%> (+0.65%) ⬆️
src/se/ 92.62% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2eecf00...60f7087. Read the comment docs.

Copy link
Collaborator Author

Mingun commented Jul 10, 2022

To solve lifetime issues, for example #332. Currently reader events can hypothetically contain an owned data, but in reality they never have. That, however, has negative implications on lifetimes.

I'll plan to make a reader event to always borrow (from buffer for buffered reader or from input for a borrowing reader). Reader event will implement an Into to the writer event, which is the current events::Event, so you will able to make transcoding as before.

You can wait with your opinion until I'll finish that work and make a PR for it

src/de/ Outdated
Copy link

@dralley dralley Jul 11, 2022

Choose a reason for hiding this comment

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

I'm mildly negative about this kind of usage because it makes the API less consistent and IMO harder to follow.

That's not to say that the implementation shouldn't be there, because it makes sense to have the implementation, but in practice when used like this it flows awkwardly when some tags use QName and others use BytesText::from_*, BytesCdata::from_*, etc. Before you only needed to look at the second "column", afterwards you have to jump back and forth between the first column and the middlle to track what is happening.

Not a blocker, just FWIW

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've made this change in order to reduce diff in one of following commits, that splits common Event into reader::Event and writer::Event. reader::Event will not have explicit public methods to construct a value (this is unnecessary for the user, if user need an event he could use writer::Event, reader events is read-only), so it will impossible to construct it in the doctests. I can move that change there

Copy link

dralley commented Jul 11, 2022

@Mingun Would you mind splitting the 7 "less interesting" commits off into a different PR, keeping 7280390 and b55aac6 here, and then then continuing your work on this PR?

I feel like it would be helpful to evaluate them alongside the additional changes you have planned. The rest are totally straightforwards.

Copy link
Collaborator Author

Mingun commented Jul 12, 2022

I think I'll just strip out controversial commits from this PR -- it is simpler.

@Mingun Mingun changed the title Add From<QName> for Start and End events and more Add <event>::borrow() and some minor stuff Jul 12, 2022
@dralley dralley merged commit d87eff8 into tafia:master Jul 12, 2022
3 checks passed
@Mingun Mingun deleted the helpers branch July 12, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants