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

Implements Lender for bytelines #8

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

vigna
Copy link

@vigna vigna commented Oct 19, 2023

This PR makes bytelines implement the LendingIterator trait from https://github.com/vigna/hrtb-lending-iterator-rs. The changes are minimal, but now standard iterator operations such as Map, Take, etc. are available. There's just a few methods, but we're working on expanding what is available. Our goal is to make everything that makes sense for a lending iterator available.

@vigna
Copy link
Author

vigna commented Oct 24, 2023

Oops. We discovered the Lender crate which is actually much more developed than ours. So I changed the PR into adding Lender support.

@vigna vigna changed the title Implements LendingIterator for bytelines Implements Lender for bytelines Oct 24, 2023
@whitfin whitfin changed the base branch from master to main December 31, 2023 17:46
@whitfin
Copy link
Owner

whitfin commented Dec 31, 2023

Hi @vigna!

I'm not against this, except that it adds a dependency I'm unfamiliar with for (what I perceive to be) little gain? Although it would be a little more efficient than into_iter() which would currently be used for this. I'm also a little concerned by this from the Lender README:

This crate was not made to be used in any sort of production code, so please, use it at your own risk (Documentation be damned! Unsafe transmutes beware!).

With this in mind, do you think inclusion is worth it? We could always hide behind a feature flag.

@vigna
Copy link
Author

vigna commented Jan 10, 2024

Well, if you consider having all Iterator functions (map, etc.) a little gain I don't think I can convince you otherwise. Lance is a bit whimsical in his documentation, but the crate is very solid and we use it in production every day. And yes, if you prefer I can but it behind a feature.

@whitfin
Copy link
Owner

whitfin commented Jan 10, 2024

Well, if you consider having all Iterator functions (map, etc.) a little gain I don't think I can convince you otherwise.

I think you misunderstood. I mean that adding Lender in your own crate is fewer than 10 lines, so including it as an additional maintenance cost inside this library likely isn't worth the trade off. It would be a little different if this was some de-facto library, but it's rather small with only ~1k downloads so I'm not sure if there's a demand.

@vigna
Copy link
Author

vigna commented Jan 10, 2024

No, you can't because of the orphan rule.

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