-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: Add windows example for Path.format #5700
Conversation
FYI: The path is not formatted if the // `root` will be used if `dir` is not specified and `name` + `ext` will be used
// if `base` is not specified
path.format({
root : "/",
ext : ".txt",
name : "file"
})
// returns
'/file.txt' |
@@ -95,6 +95,8 @@ path.extname('.index') | |||
|
|||
Returns a path string from an object, the opposite of [`path.parse`][]. | |||
|
|||
An example on \*nix: |
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.
Minor nit... rather than \*nix:
, perhaps just say An example on Posix systems:
LGTM with a nit |
Updated with the requested changes. |
LGTM |
1 similar comment
LGTM |
name : "file" | ||
}) | ||
// returns | ||
'C:\\path\\dir\\file.txt' |
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.
This line should probably also be commented or moved to the end of the previous line.
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.
Sorry, I did not follow you @mscdex . Did you mean comment out the return value?
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.
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.
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.
@mithun-daa I would say no, or at least not until #5670 lands. It might now land. I think what you've done here is fine.
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.
It landed now, so I think it's best to update this PR to use the newer style.
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.
Pushed the requested changes.
Change the *nix to Posix systems
The return value of the windows example should be commented to conform to the system wide style
8d9963c
to
0b6b6aa
Compare
PR-URL: #5700 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Landed in 54e6a8d |
@jasnell Noob question - what do you mean by commits squashed? |
@mithun-daa ... the pull request contains 3 commits, I merged them into 1 |
Thanks for clearing that up @jasnell |
Thanks for stepping up @mithun-data hope to see you more around :) |
Anytime @benjamingr - hope I can contribute more and hopefully more meaningful stuff. |
PR-URL: #5700 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Conflicts: doc/api/path.markdown
PR-URL: #5700 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #5700 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Pull Request check-list
Please make sure to review and check all of these items:
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Affected core subsystem(s)
Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)
Description of change
Please provide a description of the change here.
Add windows sample for Path.format as requested in Issue #5671