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: refactor path.format() repeated code #5673

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 12, 2016

  • 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][0]?
  • 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

Refactor repeated code into a separate function.

@Trott Trott added path Issues and PRs related to the path subsystem. lts-watch-v4.x labels Mar 12, 2016
}
return dir + posix.sep + base;
},
format: (obj) => { return format('/', obj); },
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the function name will be lost in stack traces with the arrow function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, then...will go back to the declaration style used in the rest of the object...

...and done!

@Trott
Copy link
Member Author

Trott commented Mar 12, 2016

return dir + base;
}
return dir + win32.sep + base;
format: function(pathObject) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing function name here also

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. Fixed.

@Trott
Copy link
Member Author

Trott commented Mar 12, 2016

Minor fixup pushed. New CI: https://ci.nodejs.org/job/node-test-pull-request/1908/

@benjamingr
Copy link
Member

LGTM, nice.

@benjamingr
Copy link
Member

nit: wouldn't it make more sense to use win32.sep and posix.sep everywhere instead of the literals? Not that it should block the PR, just wondering since you've refactored out of that pattern.

@@ -140,6 +140,24 @@ function normalizeStringPosix(path, allowAboveRoot) {
return res;
}

function _format(sep, pathObject) {
if (pathObject === null || typeof pathObject !== 'object') {
throw new TypeError(

Choose a reason for hiding this comment

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

Maybe keep the throws directly in path.format for better stack traces?
That would also avoid polymorphism in _format

Copy link
Member

Choose a reason for hiding this comment

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

+1 ... moving the throw up would be best.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good points about the stack trace and polymorphism as well. Rebased against master, changed according to these comments, and force pushed.

@jasnell
Copy link
Member

jasnell commented Mar 12, 2016

LGTM with a couple nits

@Trott Trott force-pushed the dry-format branch 2 times, most recently from 8627d44 to bc36ea2 Compare March 13, 2016 18:18
@Trott
Copy link
Member Author

Trott commented Mar 13, 2016

nit: wouldn't it make more sense to use win32.sep and posix.sep everywhere instead of the literals?

I had the same thought but went with the string literals because posix and win32 are defined using object literals. So they can't use posix.sep/win32.sep inside a function defined in the object literal. It could pass this.sep but that would be the only use of this in the entire file while the string literals are in multiple places. So I stuck with the string literals.

@Trott
Copy link
Member Author

Trott commented Mar 16, 2016

@mscdex
Copy link
Contributor

mscdex commented Mar 16, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Mar 16, 2016
PR-URL: nodejs#5673
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott
Copy link
Member Author

Trott commented Mar 16, 2016

Landed in 9de9a08

@Trott Trott closed this Mar 16, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
PR-URL: #5673
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

Conflicts:
	lib/path.js
@MylesBorins
Copy link
Contributor

This is blocked by other path changes /cc @nodejs/lts

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants