GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — path: indicate index of wrong resolve() parameter by sosoba · Pull Request #47660 · nodejs/node

Conversation

@sosoba

@sosoba sosoba commented Apr 21, 2023

Copy link
Copy Markdown
Contributor

Index of wrong parameter

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and tonivj5 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@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
Comment thread lib/path.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
tniessen
tniessen reviewed Apr 26, 2023
View reviewed changes

@tniessen tniessen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sosoba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/path.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/path.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@sosoba sosoba changed the title path: indicate index of wrong join() parameter path: indicate index of wrong resolve() parameter Apr 26, 2023
@deokjinkim

deokjinkim commented Apr 27, 2023

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sosoba

sosoba commented Apr 27, 2023

Copy link
Copy Markdown
Contributor Author

git rebase -i

Rebased!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@deokjinkim

deokjinkim commented Apr 28, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen

tniessen commented Apr 28, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sosoba

sosoba commented Apr 28, 2023

Copy link
Copy Markdown
Contributor Author

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

Please. My attempts failed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sosoba @tniessen
path: indicate index of wrong resolve() parameter
bea9033
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 4, 2023
tniessen
tniessen approved these changes May 4, 2023
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 4, 2023
@nodejs-github-bot

nodejs-github-bot commented May 4, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
deokjinkim
deokjinkim approved these changes May 4, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented May 4, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@deokjinkim deokjinkim added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels May 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 4, 2023
@nodejs-github-bot
nodejs-github-bot merged commit b7def8e into nodejs:main May 4, 2023
@nodejs-github-bot

nodejs-github-bot commented May 4, 2023

Copy link
Copy Markdown
Collaborator

Landed in b7def8e
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
v20.2.0 release proposal #48020
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
v18.17.0 release proposal #48694
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen approved these changes
@deokjinkim deokjinkim deokjinkim approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. 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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@sosoba @deokjinkim @tniessen @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.