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

feat: add iter/cuany #2440

Merged
merged 41 commits into from
Jun 25, 2024
Merged

feat: add iter/cuany #2440

merged 41 commits into from
Jun 25, 2024

Conversation

RidamGarg
Copy link
Contributor

@RidamGarg RidamGarg commented Jun 23, 2024

Description

What is the purpose of this pull request?

This pull request:
adds the implementation of @stdlib/iter/cuany

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

RidamGarg and others added 30 commits June 9, 2024 00:54
Signed-off-by: Philipp Burckhardt <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
@RidamGarg RidamGarg mentioned this pull request Jun 23, 2024
3 tasks
@kgryte kgryte changed the title feat: add @stdlib/iter/cuany feat: add iter/cuany Jun 23, 2024
@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Jun 23, 2024
@kgryte
Copy link
Member

kgryte commented Jun 23, 2024

@RidamGarg Would you mind updating your OP to use our PR template? You should have been prompted to use that template when opening the PR. That template includes a checkbox that you need to check in order to allow your contributions to be included in the project.

Also, it looks like CI is failing for your PR, so you'll want to address the lint and other errors before we perform a review. Thanks!

@kgryte
Copy link
Member

kgryte commented Jun 23, 2024

Also, you're attempting to open a PR from your main branch. I suggest that moving forward you use a more descriptive name. For further info, see our contributing guide, which then links to our branching guide.

@RidamGarg
Copy link
Contributor Author

Hello @kgryte , Thanks for pointing out the problems. Can I get The PR template as I am not able to find it now. For linting errors, I am able to solve them for all the other files. But not able to solve it for Readme.md file & repl.txt. Can you help me with that?

@kgryte
Copy link
Member

kgryte commented Jun 23, 2024

@kgryte
Copy link
Member

kgryte commented Jun 23, 2024

For an example, see other PRs. E.g., #2443.

@RidamGarg
Copy link
Contributor Author

Done @kgryte

@kgryte
Copy link
Member

kgryte commented Jun 25, 2024

@RidamGarg It looks like you did not follow the suggestions left in #2358 (review). For all future PRs, you need to follow our branching guidance. Any future PRs which don't won't be reviewed or accepted, as failure to follow that guidance results in significantly more work for reviewers.

For this PR, I had to refactor the entirety of the implementation. Your prior approach simply consumed the entire source iterator, created a temporary array, and then returned a new iterator. You should ask, however, what happens if the source iterator has an infinite number of values?

Additionally, you'll want to adhere more closely to our conventions: style, language (e.g., descriptions), and prior art. We place a high value on consistency.

I've tested things locally, so I'll go ahead and merge. But any future PRs in which CI does not pass due to branching issues or lint errors will not be accepted.

@kgryte kgryte merged commit d07da86 into stdlib-js:develop Jun 25, 2024
8 checks passed
@RidamGarg
Copy link
Contributor Author

Okay @kgryte, I'll keep this in mind in the future. Sorry for the inconvenience caused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants