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

test: fixed typo #44882

Merged
merged 1 commit into from Oct 26, 2022
Merged

test: fixed typo #44882

merged 1 commit into from Oct 26, 2022

Conversation

TimShilov
Copy link
Contributor

Fixed typos around the codebase, mostly in comments.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dependencies Pull requests that update a dependency file. dont-land-on-v14.x fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. npm Issues and PRs related to the npm client dependency or the npm registry. openssl Issues and PRs related to the OpenSSL dependency. v8 engine Issues and PRs related to the V8 dependency. labels Oct 3, 2022
@github-actions

This comment was marked as off-topic.

Sign in to view
@mscdex
Copy link
Contributor

mscdex commented Oct 3, 2022

Changes to upstream dependencies need to be made in the appropriate respective repositories instead of this repository.

@VoltrexKeyva
Copy link
Member

Following up to what @mscdex said, here are their relevant repositories/sources so you can contribute the proposed changes easily:

The proposed changes to the deps directory must be forwarded to their respective repositories/sources instead, shown in the following:

base64: https://github.com/aklomp/base64
cares: https://github.com/c-ares/c-ares
histogram: https://github.com/HdrHistogram/HdrHistogram_c
icu-small: https://github.com/unicode-org/icu (ICU-small is ICU but rebuilt for repeatability, verifiability, and compatibility, see the relevant part of the maintaining ICU guide here)
npm: https://github.com/npm/cli
openssl: https://github.com/openssl/openssl
undici: https://github.com/nodejs/undici
v8: https://v8.dev/docs/contribute
zlib: https://chromium.googlesource.com/chromium/src/third_party/zlib (the Zlib dependency we use is a fork by the Chromium team that applies fixes/optimizations that are not available in the standard Zlib)

The proposed changes to the test/fixtures/wpt directory must be forwarded to https://github.com/web-platform-tests/wpt

The proposed changes to the tools/node_modules/eslint directory must be forwarded to https://github.com/eslint/eslint

Although your change to the test/parallel directory is correct, you may want to either remove the other changes and leave that one only, or open a new PR.

@TimShilov TimShilov changed the title src: fix typos src: fixed typo Oct 4, 2022
@TimShilov
Copy link
Contributor Author

Sorry, I somehow missed the part of the contribution guide about the dependencies. 🤦‍♂️
Thank you for the answer, especially @VoltrexMaster for such a detailed explanation!

I have reverted all the upstream changes and this PR is now only changing test/parallel directory.

@VoltrexKeyva VoltrexKeyva added test Issues and PRs related to the tests. and removed v8 engine Issues and PRs related to the V8 dependency. openssl Issues and PRs related to the OpenSSL dependency. npm Issues and PRs related to the npm client dependency or the npm registry. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dependencies Pull requests that update a dependency file. dont-land-on-v14.x labels Oct 4, 2022
@VoltrexKeyva
Copy link
Member

VoltrexKeyva commented Oct 4, 2022 •
edited

I would recommend changing the commit message title to start with test: instead of src: since this change only affect the test directory.

@TimShilov TimShilov changed the title src: fixed typo test: fixed typo Oct 4, 2022
@VoltrexKeyva
Copy link
Member

Also be sure to remove the merge commit and rebase the other commits on top of your branch instead of adding them manually, only the latest commit is needed.

@TimShilov
Copy link
Contributor Author

@VoltrexMaster I have updated the commit message and rebased but I'm not sure if that's what you have requested. If not then please advice me on what to do here.

@VoltrexKeyva
Copy link
Member

@VoltrexMaster I have updated the commit message and rebased but I'm not sure if that's what you have requested. If not then please advice me on what to do here.

Not what I actually requested, you should use an interactive rebase to drop the older commits and only keep the latest commit (with it's commit message edited to reflect the message of this PR), and instead of merging the upstream changes on your fork to be up-to-date you should pull the upstream changes and rebase by just running git pull --rebase upstream main.

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. labels Oct 26, 2022
@aduh95 aduh95 merged commit 84044df into nodejs:main Oct 26, 2022
@aduh95
Copy link
Contributor

aduh95 commented Oct 26, 2022

Landed in 84044df, thanks for the contribution 🎉

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@TimShilov @RafaelGSS
test: fix typo in test/parallel/test-fs-rm.js
5d8f5b9 
`succesfully` -> `successfully`

PR-URL: #44882
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@TimShilov @RafaelGSS
test: fix typo in test/parallel/test-fs-rm.js
3c7ea23 
`succesfully` -> `successfully`

PR-URL: #44882
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@TimShilov @danielleadams
test: fix typo in test/parallel/test-fs-rm.js
882ec27 
`succesfully` -> `successfully`

PR-URL: #44882
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@TimShilov @danielleadams
test: fix typo in test/parallel/test-fs-rm.js
cd3e049 
`succesfully` -> `successfully`

PR-URL: #44882
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@TimShilov @danielleadams
test: fix typo in test/parallel/test-fs-rm.js
7a7dab4 
`succesfully` -> `successfully`

PR-URL: #44882
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@MoLow MoLow MoLow approved these changes

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@TimShilov @nodejs-github-bot @mscdex @VoltrexKeyva @aduh95 @lpinca @cjihrig @MoLow