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

feat: add iter/cusome #2561

Merged
merged 17 commits into from
Jul 22, 2024
Merged

feat: add iter/cusome #2561

merged 17 commits into from
Jul 22, 2024

Conversation

TheNourhan
Copy link
Contributor

@TheNourhan TheNourhan commented Jul 11, 2024 •
edited
Loading

Resolves #2334

Description

What is the purpose of this pull request?

This pull request:

  • Implementation of the iterCuSome function in the lib folder.
  • Documentation updates in the docs folder:
    • Added REPL usage examples to docs/repl.txt.
    • Created TypeScript type definitions in the docs/types folder.
  • Updated README.md.
  • Added benchmarks to the benchmark folder.
  • Added unit tests to the test folder.

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

@stdlib-js/reviewers

@kgryte
Copy link
Member

kgryte commented Jul 12, 2024

@TheNourhan I think you misunderstood the RFC. The iterator should never end until the source iterator ends. It should cumulatively test for at least n elements, forever returning true until the source iterator no longer has iterated values. See other cumulative iterators for an example.

Also, please ensure that your source implementation matches the style and conventions of other iterator packages.

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. Needs Changes Pull request which needs changes before being merged. labels Jul 12, 2024
@TheNourhan
Copy link
Contributor Author

@kgryte Sorry for the confusion, I've edited it

kgryte
kgryte requested changes Jul 14, 2024
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for continuing to work on this, @TheNourhan. This is coming along. Left a couple more comments. I suggest more closely studying cuany and updating this PR accordingly.

@kgryte
Copy link
Member

kgryte commented Jul 14, 2024

Additionally, CI failures also need to be addressed before this PR can be merged.

@TheNourhan
Copy link
Contributor Author

@kgryte I made all the changes you requested, I hope everything is fine now :)

@kgryte kgryte changed the title feat: add JavaScript implementation for iter/cusome feat: add iter/cusome Jul 18, 2024
@TheNourhan
Copy link
Contributor Author

If there something I can improve to move forward with the merger? @kgryte

@kgryte kgryte removed the Needs Changes Pull request which needs changes before being merged. label Jul 22, 2024
kgryte
kgryte approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for working on this, @TheNourhan. I made a number of changes which you'll want to keep in mind for future PRs. The main thing is being consistent with code elsewhere in the project.