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

Add iterator #320

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add iterator #320

wants to merge 5 commits into from

Conversation

Sytten
Copy link
Contributor

@Sytten Sytten commented May 27, 2024

This is my first attempt at #318

Couple of points before reviewing:

  • I decided to base the implementation on ArrayBuffer since Iterable and Iterator are not JS types and I didn't feel comfortable adding them to the Type enum
  • Still unsure if I like the name of the trait IntoJsIter since it's not really an Into trait like we generally see in rust, but it's similar in spirit to IntoJsFunc
  • The parameters of IntoJsIter are a bit strange with the position. I use the position in my util crate https://github.com/DelSkayn/rquickjs/pull/319/files#diff-653c6889323b6067b2be818f8ff00f785927ae3beaaeaba4a034feb5ea154d4eR133 since I do some magic with the This to access the parent class to avoid copying the values and my function is stateless so I need some indication on where we are. But we might want to remove it from the trait and move it to IterFn and IterFnMut

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2024

Codecov Report

Attention: Patch coverage is 81.02981% with 70 lines in your changes are missing coverage. Please review.

Project coverage is 68.75%. Comparing base (304db5d) to head (8b80197).

Files Patch % Lines
core/src/value/iterator/iterable.rs 70.33% 35 Missing ⚠️
core/src/value/iterator.rs 88.69% 26 Missing ⚠️
core/src/value/iterator/into_iter.rs 57.14% 9 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
+ Coverage   68.31%   68.75%   +0.43%     
==========================================
  Files          83       86       +3     
  Lines       12237    12606     +369     
==========================================
+ Hits         8360     8667     +307     
- Misses       3877     3939      +62     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sytten Sytten mentioned this pull request Jun 4, 2024
@Sytten
Copy link
Contributor Author

Sytten commented Jun 6, 2024

Gentle ping @DelSkayn, whenever you have time I just want to avoid it getting stale :)

@Sytten Sytten mentioned this pull request Jan 7, 2025
5 tasks
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