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

doc: make path.relative stricter #13714

Closed
wants to merge 2 commits into from
Closed

Conversation

DuanPengfei
Copy link
Contributor

think of this situation: you are current under root path and you do

path.relative('../../../../../x', '../../y');

path.relative can't fictitious a path for the result, so it's based
on current working directory.

Checklist
Affected core subsystem(s)

doc, path

think of this situation: you are current under root path and you do

```
path.relative('../../../../../x', '../../y');
```

`path.relative` can't fictitious a path for the result, so it's based
on current working directory.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Jun 16, 2017
doc/api/path.md Outdated
If `from` and `to` each resolve to the same path (after calling `path.resolve()`
on each), a zero-length string is returned.
The `path.relative()` method returns the relative path from `from` to `to` based
on current working directory. If `from` and `to` each resolve to the same path
Copy link
Contributor

Choose a reason for hiding this comment

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

s/on/on the/

@gibfahn
Copy link
Member

gibfahn commented Jun 16, 2017

This fixes #13683

@DuanPengfei
Copy link
Contributor Author

/ping @refack
Does this PR can be landed?

@refack
Copy link
Contributor

refack commented Jun 19, 2017

/ping @refack
Does this PR can be landed?

Yes.
Linter: https://ci.nodejs.org/job/node-test-linter/9937/

refack pushed a commit to refack/node that referenced this pull request Jun 19, 2017
PR-URL: nodejs#13714
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Jun 19, 2017

Landed in 6e69421
@DuanPengfei Congratulation on your first contribution
before:
image
After
image

@refack refack closed this Jun 19, 2017
addaleax pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #13714
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
addaleax pushed a commit that referenced this pull request Jun 21, 2017
PR-URL: #13714
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
DuanPengfei added a commit to souche-koumakan/node that referenced this pull request Jun 22, 2017
Throw an error `ERR_UNSUPPORTED_PLATFOMR` when direct use
`path.posix.resolve` on Windows or direct use `path.win32.resolve`
on *nix.

Update docs, list win32 functions do not support direct use on
*nix and posix functions do not support direct use on Windows.

Update tests, only run current platform type test.

Fixes: nodejs#13683
Refs: nodejs#13714
MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
PR-URL: #13714
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

I.ve backported to v6.x. If it shouldn't have been backported let me know and I'll rebase it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants