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

Replace fallible-iterator with tryiterator #620

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

khuey
Copy link
Contributor

@khuey khuey commented Apr 13, 2022

As we've discussed over email. Just looking for feedback here, I have to release tryiterator on crates.io before we merge this.

@philipc
Copy link
Collaborator

philipc commented Apr 14, 2022

These changes are all look trivial, which is good.

However, it appears that something like AttrsIter::count will silently discard any errors. Do you have any way to address that? This was the original reason we started using FallibleIterator.

Have you sort feedback from the larger Rust community about tryiterator?

@khuey
Copy link
Contributor Author

khuey commented Apr 14, 2022

These changes are all look trivial, which is good.

However, it appears that something like AttrsIter::count will silently discard any errors. Do you have any way to address that? This was the original reason we started using FallibleIterator.

It would be straightforward to add a TryIterator::count_ok.

Have you sort feedback from the larger Rust community about tryiterator?

I started to ask people for their opinions privately this week. I'm in no hurry to merge this.

@philipc
Copy link
Collaborator

philipc commented Apr 14, 2022

It would be straightforward to add a TryIterator::count_ok.

Right, but it would need a lint to determine the cases where count was wrong.

@khuey
Copy link
Contributor Author

khuey commented Apr 14, 2022

Another option that was suggested to me is to add an intentionally conflicting TryIteratorExt::count so that iter.count() won't work without qualifying which trait is being used.

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