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 array/base/count-same-value-zero #1384

Merged
merged 6 commits into from
Feb 28, 2024
Merged

feat: add array/base/count-same-value-zero #1384

merged 6 commits into from
Feb 28, 2024

Conversation

AgPriyanshu18
Copy link
Contributor

@AgPriyanshu18 AgPriyanshu18 commented Feb 25, 2024 •
edited by kgryte
Loading

Resolves #1326.

Description

What is the purpose of this pull request?

This pull request:

  • add count-same-value-zero functionality to the array packages of stdlib

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

@AgPriyanshu18
feat: added @stdlib/array/base/count-same-value-zero
3c9e0d5 
count the number of elements that are equal to a given value in an array

Fixes stdlib-js#1326
stdlib-bot
stdlib-bot reviewed Feb 25, 2024
View reviewed changes
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. 🏃 💨

Jaysukh-409
Jaysukh-409 reviewed Feb 25, 2024
View reviewed changes
Copy link
Member

@Jaysukh-409 Jaysukh-409 left a comment

Choose a reason for hiding this comment

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

Once this changes are done I think this PR will be ready to merge.

@AgPriyanshu18 @Jaysukh-409
Apply suggestions from code review
218106a 
Co-authored-by: Jaysukh Makvana <111515433+Jaysukh-409@users.noreply.github.com>
Signed-off-by: AgPriyanshu18 <113460573+AgPriyanshu18@users.noreply.github.com>
@kgryte kgryte changed the title feat: added @stdlib/array/base/count-same-value-zero feat: add array/base/count-same-value-zero Feb 25, 2024
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Review A pull request which needs code review. Utilities Issue or pull request concerning general utilities. labels Feb 25, 2024
@kgryte
Copy link
Member

kgryte commented Feb 25, 2024

@AgPriyanshu18 This PR is missing a docs/repl.txt file, which needs to be added before this PR can be reviewed.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Feb 25, 2024
Jaysukh-409
Jaysukh-409 reviewed Feb 26, 2024
View reviewed changes
lib/node_modules/@stdlib/array/base/count-same-value-zero/lib/main.js
* @example
* var x = [ 0, 0, 1, 0, 1 ];
*
* var n = countSameValueZero( x, 1 );
Copy link
Member

Choose a reason for hiding this comment

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

Before using countSameValueZero you have to import it. You can do it as shown below.

var countSameValueZero = require( '@stdlib/array/base/is-same-value-zero' );

Copy link
Contributor Author

@AgPriyanshu18 AgPriyanshu18 Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It was a commented-out example that's why I didn't import the package.

Copy link
Member

Choose a reason for hiding this comment

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

While linting we also check the example code syntax also and that's why it should be correct.

@AgPriyanshu18
Copy link
Contributor Author

@kgryte I have added the file and have made suggested changes.

Planeshifter
Planeshifter approved these changes Feb 28, 2024
View reviewed changes
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.

Let's get this merged; will do so after CI has passed. Thanks for your contribution!

lib/node_modules/@stdlib/array/base/count-same-value-zero/lib/main.js Outdated Show resolved Hide resolved
lib/node_modules/@stdlib/array/base/count-same-value-zero/lib/main.js Outdated Show resolved Hide resolved
@Planeshifter
Update lib/node_modules/@stdlib/array/base/count-same-value-zero/lib/…
3aed444 
…main.js

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter
Update lib/node_modules/@stdlib/array/base/count-same-value-zero/lib/…
678ea55 
…main.js

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
@Planeshifter Planeshifter merged commit 1ed81b7 into stdlib-js:develop Feb 28, 2024
6 of 7 checks passed
@AgPriyanshu18 AgPriyanshu18 deleted the feature/count-same-value-zero branch February 28, 2024 05:57
@kgryte
Copy link
Member

kgryte commented Feb 29, 2024

@Planeshifter Why did this one get merged when CI was failing?

@Planeshifter
Copy link
Member

@kgryte don't recall exactly why i didn’t notice, but I fixed the require path in a subsequent commit to make sure tests pass. Bug was only due to require path in example code that got inserted during review process.

One downside of requiring signed commits is that one regularly has to bypass branch protection rules and there is not an easy visual cue without reading it fully of whether it's blocked to CI failures or just due to commits not being signed.

kgryte added a commit that referenced this pull request Mar 16, 2024
@kgryte
docs: clean-up documentation and examples and rename folder
422cebf 
Ref: #1384
Ref: 1ed81b7
@AgPriyanshu18 AgPriyanshu18 mentioned this pull request Mar 21, 2024
Closed
6 tasks
@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
Reviewers

@stdlib-bot stdlib-bot stdlib-bot left review comments

@Jaysukh-409 Jaysukh-409 Jaysukh-409 left review comments

@Planeshifter Planeshifter Planeshifter approved these changes

Assignees
No one assigned
Labels
Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. Utilities Issue or pull request concerning general utilities.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: add @stdlib/array/base/count-same-value-zero
5 participants
@AgPriyanshu18 @kgryte @Planeshifter @stdlib-bot @Jaysukh-409