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

path: fix win32 parse() #5484

Closed
wants to merge 1 commit into from
Closed

Conversation

zbinlin
Copy link
Contributor

@zbinlin zbinlin commented Feb 29, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

path

Description of change

In Node v5.7.0, use path.win32.parse to parse '/foo/bar' will returns '' for root, but not '/'(v5.6.0).

Fix path.win32.parse("/foo/bar") retuns `{root: '' ...}`(v5.7.0),
but not `{root: '/' ...}`(v5.6.0).
@mscdex mscdex added windows Issues and PRs related to the Windows platform. path Issues and PRs related to the path subsystem. labels Feb 29, 2016
@mscdex
Copy link
Contributor

mscdex commented Feb 29, 2016

@mscdex mscdex added this to the 5.7.1 milestone Feb 29, 2016
@mscdex mscdex mentioned this pull request Feb 29, 2016
5 tasks
@mscdex
Copy link
Contributor

mscdex commented Feb 29, 2016

LGTM

1 similar comment
@silverwind
Copy link
Contributor

LGTM

silverwind pushed a commit that referenced this pull request Feb 29, 2016
Fix path.win32.parse("/foo/bar") retuns `{root: '' ...}`(v5.7.0),
but not `{root: '/' ...}`(v5.6.0).

PR-URL: #5484
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind
Copy link
Contributor

Thanks! Landed in 4717ea9.

@silverwind silverwind closed this Feb 29, 2016
@zbinlin zbinlin deleted the fix-path-parse-win32 branch March 1, 2016 03:34
@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

@mscdex ... is this related to your recent path changes (and therefore not relevant to v4) ?

Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
Fix path.win32.parse("/foo/bar") retuns `{root: '' ...}`(v5.7.0),
but not `{root: '/' ...}`(v5.6.0).

PR-URL: #5484
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Roman Reiss <me@silverwind.io>

Conflicts:
	test/parallel/test-path-parse-format.js
@mscdex
Copy link
Contributor

mscdex commented Mar 2, 2016

@jasnell Correct.

@MylesBorins
Copy link
Contributor

Adding a don't land flag. If we decide to bring back those path changes we can modify this

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

Successfully merging this pull request may close these issues.

5 participants