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.

Sign up for GitHub

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

Jump to bottom

Test specializations #782

Merged
merged 3 commits into from Oct 15, 2023
Merged

Test specializations #782

merged 3 commits into from Oct 15, 2023

Conversation

phimuemue
Copy link
Member

We have more and more specializations, but so far they are not vastly tested for iterators that have already been advanced. This PR alters test_specializations.

@phimuemue
check_specialized only needed within function
eabc235
@phimuemue
Test specializations for next'ed iterators
fbe6e32
@phimuemue
Add TODO
5d88ded 
process_results is as of now tested separately because the corresponding
iterator is not exposed nicely, and because it is not `Clone`.

However, the respective tests already diverged. Instead of fixing
process_results, we should strive to uniformly use test_specializations
for ProcessResults, too.
@Philippe-Cholet
Copy link
Member

I worked some time ago on a (maybe too complex) way to test specializations (draft: #756 (comment)) before I split focus on other things.
Here, 5 as limit might be too little, at least fior some iterators, I would suggest up to 10.

@phimuemue
Copy link
Member Author

phimuemue commented Oct 15, 2023 •
edited

@Philippe-Cholet Can you merge this? I‘ll raise the limit to ten when I have time.

Side note: tbh I just filed this as a PR to have the CI/Tests/Checks running, but it seems I cannot merge myself anymore (as it was the case back then with bors). Am I doing something wrong?

@phimuemue phimuemue added this pull request to the merge queue Oct 15, 2023
Merged via the queue into rust-itertools:master with commit f00e3ae Oct 15, 2023
8 checks passed
@phimuemue phimuemue deleted the test_specializations branch October 15, 2023 10:29
@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Oct 15, 2023 •
edited

@phimuemue All the maintainer part is new to me but I guess we can not merge a PR (except jswrenn I suppose as he is an owner) especially if it has a commit we wrote without explicit approval, something like that. I think it would have work if you approved your own changes.

@jswrenn jswrenn mentioned this pull request Nov 14, 2023
Merged
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Philippe-Cholet Philippe-Cholet Philippe-Cholet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@phimuemue @Philippe-Cholet @jswrenn