path: assert inputs are strings by mscdex · Pull Request #5348 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This makes input checks more consistent across the path functions.

mscdex added semver-major

PRs that contain breaking changes and should be released in the next major version.

path

Issues and PRs related to the path subsystem.

labels

Feb 21, 2016

CI is all green except a flaky test.

Should these changes be accompanied by a doc update?

LGTM, I suppose a doc addition stating the input type would be nice, yeah.

silverwind


Return the extension of the path, from the last '.' to end of string
in the last portion of the path. If there is no '.' in the last portion
of the path or the first character of it is '.', then it returns
an empty string. Examples:
an empty string. `path` must be a string.

Choose a reason for hiding this comment

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

Maybe do it like in the buffer docs?

## path.extname(path)

* `path` {String}

description...

Choose a reason for hiding this comment

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

The docs should definitely be consistent but that can be done in a separate PR, I think. The entire path.markdown file is inconsistent with the other docs it appears.

Choose a reason for hiding this comment

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

Yeah, I realized we're pretty inconsistent with argument description all over, so disregard my comment.

This commit makes input type checking consistent across all path functions.

PR-URL: nodejs#5348

mscdex added a commit that referenced this pull request

Mar 18, 2016

This commit makes input type checking consistent across all path functions.

PR-URL: #5348 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Roman Reiss me@silverwind.io

mscdex deleted the path-assert-inputs branch

March 18, 2016 03:24

This change is breaking the testing suite of both eslint + stylus

I have documented this over at eslint already: eslint/eslint#5678 (comment)
Will follow up with stylus afterwards

So it looks like the regression is some what similar to what we saw in #5244

Basically anyone who was passing null / undefined to path in the past would get an empty string back, now Node throws.

I have sent PR's to both eslint + stylus, but I can imagine that the pattern of passing undefined into a path function probably exists all over the place.

This is likely to be VERY breaking in the ecosystem (we only smoke test 48 modules, 2 of which were affected).

@nodejs/ctc assuming this is a change that we want to keep in v6, what is the best way to avoid a ton of breakages in the ecosystem?

I think the right thing to do would likely go back to the behavior of returning an empty string on null/undefined

This has come up before (#1215). We should fix for versions prior to 6, but we should use this new behavior in 6+.

so this is working as expected prior to v6. I still thinking that supplying '.' for undefined is the right thing to do. Perhaps with a deprecation message? This will be a bit out of left field for individuals moving to six with no notice, and we have no guarantee that some of the breaking modules are still maintained.

edit: I've played around with "fixing" this a little bit and I am left at a bit of an impasse. Obviously we don't want to break stuff, but being consistent on this is going to be hard. What do we do for path.join where the first argument is not a string, and the second is. In a way being strict about validating strings makes a ton of sense in this case.

Perhaps we should only be returning an empty string or '.' for functions that had one argument, but drawing that line seems pretty arbitrary.

Looking forward to hearing some more opinions

The main purpose of this PR was to create consistency. I don't have a strong feeling about which direction, but I think it should be that either all functions type check or they don't (and do implicit string conversions or whatever other solution).

jasnell added a commit that referenced this pull request

Apr 26, 2016