Skip to content

feat: add array/base/count-if #1372

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

Merged

Conversation

bad-in-coding
Copy link
Contributor

@bad-in-coding bad-in-coding commented Feb 24, 2024

Resolves #1324 .

Description

What is the purpose of this pull request?

This pull request adds the package @stdlib/array/base/count-if.

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

Copy link
Contributor

@stdlib-bot stdlib-bot left a comment

Choose a reason for hiding this comment

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

👋 Hi there! 👋

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

@Planeshifter Planeshifter self-requested a review February 24, 2024 20:27
@kgryte kgryte changed the title feat: added array/base/count-if feat: add array/base/count-if Feb 24, 2024
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Utilities Issue or pull request concerning general utilities. Needs Review A pull request which needs code review. labels Feb 24, 2024
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Thank you for taking on this package!

We should change the function signature for the predicate function to not only receive the value and this context, but also the array index and array reference itself. See comment on the main.js file. This will match the project convention and provide more flexibility to users.

@bad-in-coding bad-in-coding force-pushed the feature/array/base/count-if branch from 296d2ad to 64c19d4 Compare February 25, 2024 06:36
@bad-in-coding
Copy link
Contributor Author

@Planeshifter I have fixed the issues :)

Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Implementation looks great; the one thing we could improve a bit are the TypeScript definitions. Would you mind taking a look at this, please? Thanks in advance!

@bad-in-coding
Copy link
Contributor Author

bad-in-coding commented Mar 2, 2024

@Planeshifter done 👍

Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Great, thank you! Let's get this landed! Made some minor doc changes (aiming to use the 80 character line width in repl.txt more fully and adding spaces in the test.ts files per our code style) and will merge once CI passes.

@Planeshifter
Copy link
Member

@Ut-the-pro There were a few more small lint errors that had to be fixed such as trailing spaces; if you are using a code editor such as VSCode or Sublime Text, it's a good idea to install extensions for ESLint and editorconfig to automatically format your code on save, Also, don't forget to run the stdlib make recipes for linting and testing during development after setting up your dev environment with make init.

But this should be ready to land finally, thanks again! 🎉

@Planeshifter Planeshifter merged commit ecd3518 into stdlib-js:develop Mar 2, 2024
@bad-in-coding bad-in-coding deleted the feature/array/base/count-if branch March 4, 2024 04:35
bad-in-coding added a commit to bad-in-coding/stdlib that referenced this pull request Mar 7, 2024
PR-URL: stdlib-js#1372
Closes: stdlib-js#1324

Signed-off-by: Utkarsh <[email protected]>
Signed-off-by: Philipp Burckhardt <[email protected]>
Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]> 
Reviewed-by: Athan Reines <[email protected]>
@Planeshifter Planeshifter removed the Needs Review A pull request which needs code review. label Mar 26, 2024
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. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: add @stdlib/array/base/count-if
4 participants