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

Remove requirement to have Reader when decoding attributes and write anything converted to Event #760

Merged
merged 3 commits into from
Jun 23, 2024

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Jun 16, 2024

This PR includes two changes, which both related to my work of splitting event into two event classes -- for reading and for writing. Events for reading then would only borrow data and this will help to fix #332 and also open doors to no_std reader (not sure why it would be needed although).

The first commit just removes unused import that I noticed while working on this.

The second commit relaxes requirements for decoding values from Attribute. If you created attribute for writing and then for whatever reason wants to read its value back, you need a Reader which you does not have. This commit fixes that.

The third commit makes writing methods take Into<Event> instead of AsRef<Event>. The former trait give a lesser abilities then Into. It allows you to pass your own struct to write, but you need to have event in your struct to be able to take a ref to it. Into would allow you to construct event on demand and also would allow to directly pass reader event to the writer method that would require writer event if (when?) event splitting will be implemented. In any case, I think, that taking Into is better.

If there are any objections, then unnecessary commits can be removed from PR.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2024

Codecov Report

Attention: Patch coverage is 68.18182% with 14 lines in your changes missing coverage. Please review.

Project coverage is 60.30%. Comparing base (7558577) to head (55f7aa1).
Report is 43 commits behind head on master.

Files Patch % Lines
src/events/attributes.rs 0.00% 6 Missing ⚠️
benches/macrobenches.rs 0.00% 4 Missing ⚠️
examples/read_nodes.rs 0.00% 3 Missing ⚠️
examples/custom_entities.rs 0.00% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
- Coverage   61.81%   60.30%   -1.52%     
==========================================
  Files          41       41              
  Lines       16798    16191     -607     
==========================================
- Hits        10384     9764     -620     
- Misses       6414     6427      +13     
Flag Coverage Δ
unittests 60.30% <68.18%> (-1.52%) ⬇️

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 will allow decode them even if Reader does not exists, for example,
when you only write events.
…wnership

This change will allow to feed to the writer different structs which, otherwise,
would be impossible if are not holds an `Event` inside your struct. In the future
the Event may be split into `reader::Event` and `writer::Event` and the reader
variant may evolve to be borrow-only.
@Mingun
Copy link
Collaborator Author

Mingun commented Jun 23, 2024

Usability of the last commit can be demonstrated in #766. There I plan to introduce conception of low-level RawReader which produces RawEvents. Those are current Reader and Event types. Also I plan introduce conception of high-level Reader which would produce new Events.

I plan to make RawEvent borrow-only, i.e. they will contain &[u8], and Event will contain Cow<[u8]>, because new Reader would produce events from different files. However, I'd want to be able to write both types of events using the same Writer (there are no reasons to have RawWriter). Because Event will not contain RawEvent inside, it will not be possible to implement AsRef<RawEvent> for it.

@Mingun Mingun merged commit 4674244 into tafia:master Jun 23, 2024
6 checks passed
@Mingun Mingun deleted the in-mind-of-write branch June 23, 2024 14:53
@dralley
Copy link
Collaborator

dralley commented Jun 23, 2024

However, I'd want to be able to write both types of events using the same Writer (there are no reasons to have RawWriter). Because Event will not contain RawEvent inside, it will not be possible to implement AsRef for it.

@Mingun is it possible to go the opposite direction, i.e. have RawEvent implement Into<Event>?

@Mingun
Copy link
Collaborator Author

Mingun commented Jun 23, 2024

Yes, it will be done that way.

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.

Lifetime on Attributes is wrong
3 participants