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

util: Use primordials.ArrayPrototypeIndexOf instead of mutable method #48586

Merged
merged 1 commit into from
Jul 15, 2023
Merged

util: Use primordials.ArrayPrototypeIndexOf instead of mutable method #48586

merged 1 commit into from
Jul 15, 2023

Conversation

DaisyDogs07
Copy link
Contributor

@DaisyDogs07 DaisyDogs07 commented Jun 28, 2023

Found a part of identicalSequenceRange that uses a mutable method
Changed it to use the primordial instead

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Jun 28, 2023
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Thanks! Can you amend the commit message to comply with our guidelines? Something like util: use `primordials.ArrayPrototypeIndexOf` instead of mutable method

@DaisyDogs07 DaisyDogs07 changed the title Use primordial ArrayPrototypeIndexOf in identicalSequenceRange util: Use primordial ArrayPrototypeIndexOf in identicalSequenceRange Jun 28, 2023
@DaisyDogs07 DaisyDogs07 changed the title util: Use primordial ArrayPrototypeIndexOf in identicalSequenceRange util: Use primordials.ArrayPrototypeIndexOf in identicalSequenceRange Jun 28, 2023
@DaisyDogs07
Copy link
Contributor Author

Sorry about that. First time making a pull request or anything for something big :/

@DaisyDogs07 DaisyDogs07 changed the title util: Use primordials.ArrayPrototypeIndexOf in identicalSequenceRange util: Use primordials.ArrayPrototypeIndexOf instead of mutable method Jun 28, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jun 29, 2023

I can see you changed the PR title, but unfortunately that's not what's needed: you need to amend the commit message, and then force push to your PR branch. Please make sure to also follow this rule:

* be entirely in lowercase with the exception of proper nouns, acronyms, and
the words that refer to code, like function/variable names

Let me know if you need help with that, I can do it for you if that's more convenient.

@DaisyDogs07
Copy link
Contributor Author

DaisyDogs07 commented Jun 29, 2023

Unfortunately im new to PRs and i dont know how to force push something. may i ask if you could do it for me?

I believe you forgot to use the primordial `ArrayPrototypeIndexOf` in `identicalSequenceRange`
i dont know if this was intentional but im fixing it anyway :p
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit 4a26e2d into nodejs:main Jul 15, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jul 15, 2023

Landed in 4a26e2d, thanks for the contribution 🎉

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #48586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
ruyadorno pushed a commit that referenced this pull request Sep 12, 2023
PR-URL: #48586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 13, 2023
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants