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

doc: update path.format description and examples #10046

Closed
wants to merge 1 commit into from

Conversation

anoff
Copy link
Contributor

@anoff anoff commented Dec 1, 2016

This came up while talking to @sam-github during the code-and-learn

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change
  • removed pseudo-code
  • rename pathObject to options to improve readability
  • added info on which properties have priority
  • modified examples to show ignored properties

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@@ -166,12 +166,12 @@ path.extname('.index')

A [`TypeError`][] is thrown if `path` is not a string.

## path.format(pathObject)
## path.format(options)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@anoff anoff Dec 2, 2016

Choose a reason for hiding this comment

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

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.

@Trott
Copy link
Member

Trott commented Dec 2, 2016

@nodejs/documentation

path.format({
root: '/',
base: 'file.txt'
base: 'file.txt',
ext: 'ignored'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a TAB character here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow @thefourtheye thanks for the thorough check. Had my editor configured wrong from a recent project.

@sam-github
Copy link
Contributor

LGTM, thanks a lot.

@sam-github
Copy link
Contributor

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
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 8, 2016
* 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>
@Trott
Copy link
Member

Trott commented Dec 8, 2016

Landed in 2d0ce51

@Trott Trott closed this Dec 8, 2016
Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
* 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>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
MylesBorins pushed a commit that referenced this pull request Jan 16, 2017
* 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>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
* 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>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants