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

path: indicate index of wrong resolve() parameter #47660

Merged
merged 1 commit into from
May 4, 2023
Merged

path: indicate index of wrong resolve() parameter #47660

merged 1 commit into from
May 4, 2023
+2 −3

Conversation

sosoba
Copy link
Contributor

@sosoba sosoba commented Apr 21, 2023

Index of wrong parameter

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels Apr 21, 2023
@sosoba sosoba changed the title [impr] Indicate index of wrong join() parameter node:path indicate index of wrong join() parameter Apr 21, 2023
@sosoba sosoba changed the title node:path indicate index of wrong join() parameter path: indicate index of wrong join() parameter Apr 21, 2023
tniessen
tniessen reviewed Apr 21, 2023
View reviewed changes
lib/path.js Outdated Show resolved Hide resolved
tniessen
tniessen reviewed Apr 26, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen 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 the contribution @sosoba. Could you please remove the merge commits from your branch patch-1, for example, by rebasing locally?

Also, while the title of the PR says that this affects join(), it seems to me that the diff targets resolve() but not join().

lib/path.js Outdated Show resolved Hide resolved
lib/path.js Outdated Show resolved Hide resolved
@sosoba sosoba changed the title path: indicate index of wrong join() parameter path: indicate index of wrong resolve() parameter Apr 26, 2023
@deokjinkim
Copy link
Contributor

@sosoba Could you please squash 8 commits to 1 commit using git rebase -i? And please follow Commit message guidelines. For example, commit title needs to be started with subsystem like path: show index of wrong resolve() parameter.

https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines

@sosoba
Copy link
Contributor Author

sosoba commented Apr 27, 2023

git rebase -i

Rebased!

@deokjinkim
Copy link
Contributor

@sosoba I mean squash(not rebase) in git. Squash in git means to combine multiple commits into one. You can squash using git rebase -i command. Here is guide for squash in git.

https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec

And need to follow commit message guideline I mentioned above.

@tniessen
Copy link
Member

@sosoba You will need to checkout your branch locally and remove the merge commits. You can optionally squash the non-merge commits on this branch, but in any case, the commit message of the first commit should follow our guidelines (and match the PR title).

If you would like us to do this for you, just let me know.

@sosoba
Copy link
Contributor Author

sosoba commented Apr 28, 2023

If you would like us to do this for you, just let me know.

Please. My attempts failed.

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51622/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51624/

@deokjinkim deokjinkim added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels May 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit b7def8e into nodejs:main May 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in b7def8e

targos pushed a commit that referenced this pull request May 12, 2023
@sosoba @targos
path: indicate index of wrong resolve() parameter
7222f9d 
PR-URL: #47660
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
@targos targos mentioned this pull request May 15, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@sosoba @danielleadams
path: indicate index of wrong resolve() parameter
8f7189f 
PR-URL: #47660
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@sosoba @MoLow
path: indicate index of wrong resolve() parameter
b34eaf3 
PR-URL: nodejs#47660
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deokjinkim deokjinkim deokjinkim approved these changes

@tniessen tniessen tniessen 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. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sosoba @deokjinkim @tniessen @nodejs-github-bot