doc: update path.format description and examples
#10046
Conversation
nodejs-github-bot
added
doc
imyller
added
the
code-and-learn
| @@ -166,12 +166,12 @@ path.extname('.index') | |||
|
|
|||
| A [`TypeError`][] is thrown if `path` is not a string. | |||
|
|
|||
| ## path.format(pathObject) | |||
| ## path.format(options) | |||
There was a problem hiding this comment.
Thanks for tackling this issue in the docs. I find the algorithmic explanation only marginally better than reading the code directly, so I'm glad to see an effort to improve this portion of the path doc.
The names of function arguments in the docs match the names in the code. So if we're going to change pathObject to options here, we probably want to change it in lib/path.js as well.
That said, I don't think options is better than pathObject. This is not an object containing options; it's an object containing data representing a path. I think pathObject is better.
There was a problem hiding this comment.
I see your point there with it not being consistent to the code. The reason behind the change was more about having shorter mentions in the description and a (possibly) more consistent user experience when reading the docs. But you're right that it's not actually options.
edit: Since the whole thing is a kind of confusing matter how do you feel about linking to the actual code to look it up? Or is the general idea that people trying to figure out how an edge case might be handled to just try it or go through the code themselves? Linking to code lines sounds like something that will just result in dead/wrong links in a couple of versions.
|
@nodejs/documentation |
| path.format({ | ||
| root: '/', | ||
| base: 'file.txt' | ||
| base: 'file.txt', | ||
| ext: 'ignored' |
There was a problem hiding this comment.
Is there a TAB character here?
There was a problem hiding this comment.
wow @thefourtheye thanks for the thorough check. Had my editor configured wrong from a recent project.
|
LGTM, thanks a lot. |
|
Please squash these into a single commit, and force push the branch. |
* removed pseudo-code * added info on which properties have priority * modified examples to show ignored properties
* removed pseudo-code * added info on which properties have priority * modified examples to show ignored properties PR-URL: nodejs#10046 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
|
Landed in 2d0ce51 |
* removed pseudo-code * added info on which properties have priority * modified examples to show ignored properties PR-URL: #10046 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
* removed pseudo-code * added info on which properties have priority * modified examples to show ignored properties PR-URL: #10046 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
* removed pseudo-code * added info on which properties have priority * modified examples to show ignored properties PR-URL: #10046 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
* removed pseudo-code * added info on which properties have priority * modified examples to show ignored properties PR-URL: #10046 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
This came up while talking to @sam-github during the
code-and-learnChecklist
Affected core subsystem(s)
doc
Description of change
pathObjecttooptionsto improve readability