| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
@nodejs/documentation |
Sorry, something went wrong.
There was a problem hiding this comment.
Is there a TAB character here?
Sorry, something went wrong.
There was a problem hiding this comment.
wow @thefourtheye thanks for the thorough check. Had my editor configured wrong from a recent project.
Sorry, something went wrong.
|
LGTM, thanks a lot. |
Sorry, something went wrong.
|
Please squash these into a single commit, and force push the branch. |
Sorry, something went wrong.
* 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>
* 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>
| Back | FazBrowse Home | New Git URL |
This came up while talking to @sam-github during the code-and-learn
Checklist
Affected core subsystem(s)
doc
Description of change