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

Attempt to use common trait between variants #207

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

michalfita
Copy link
Contributor

Important

This is in the ideation stage, not supposed to be merged without any real life proof of sense.

I made attempt to create a trait shared between lossy and lossless variants of Repository, but this has following flaws:

  • Interface over lossy variant works nice as we can refer to member; I tried Cow<> but now dealing with fact Paragraph::get() may fail become a problem (unwrap() is plain wrong).
  • Use Cow<> is going to work for some cases, but may be a problem for some others, especially if wrapped in Option<>
  • For optional fields use of .ok() turns potential errors of late parsing into None, I have doubts if it's acceptable.

Problems with lossless structure:

  • Paragraphs are copied out of Deb822, there's not reference to them
  • If lossless::Repositories store Deb822, I can only create lossless::Repository from these paragraphs, there's not way to return reference to them (that's different than in lossy variant).

Maybe I'm heading in wrong direction (no modifications/setting taken into account yet), as this route gives me identical API of both variants, but with problems. Maybe the role of lossless variant should be to produce out lossy variant and accept one to perform modifications.

Can we discuss pros and cons?

@jelmer
Copy link
Owner

jelmer commented Mar 17, 2025

Hey, this is still marked as draft - is this ready?

@michalfita
Copy link
Contributor Author

Simple answer is: somehow.
Explanation: I stuck with Signature and reference to it via Cow<> (or not). It happen when my time on this has ended, so the PR doesn't even have my latest changes with more tests and examples. As I progressed with testing I had to amend synopses to actually make sens for users. I'd like to finish this, but ATM I can't give any promises - this is currently only a side hustle (even if derived from work I did for work).
As unhappy as I am I have to say patience required. This was more mentally consuming that I initially anticipated, but I believe I cooked something interesting here.

@michalfita
Copy link
Contributor Author

OK, I pushed my latest state of unfinished work. It can get rough review about the idea and the premise, but it's not in the mergeable state yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants