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: path.format provide more examples #5838

Closed

Conversation

eversojk
Copy link
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

path

Description of change

Added examples for all cases of path.format for V5.9.0 on Linux.

Fixes #5747

@eversojk
Copy link
Contributor Author

I've run the tests to get into the habit of doing so and seem to be getting an error unrelated to any of my changes. If someone could confirm they are seeing something similar I'd be happy to create an issue.

make jslint
make[1]: Entering directory '/home/john/projects/node'
./node tools/eslint/bin/eslint.js benchmark lib src test tools/doc \
        tools/eslint-rules --rulesdir tools/eslint-rules

/home/john/projects/node/benchmark/http_simple_auto.js
  16:13  error  Strings must use singlequote  quotes
  35:19  error  Strings must use singlequote  quotes
  44:12  error  'i' is not defined            no-undef
  44:19  error  'i' is not defined            no-undef
  44:26  error  'i' is not defined            no-undef
  45:25  error  'i' is not defined            no-undef
  77:10  error  'i' is not defined            no-undef
  77:37  error  'i' is not defined            no-undef
  77:46  error  'i' is not defined            no-undef
  78:28  error  'i' is not defined            no-undef
  78:38  error  'i' is not defined            no-undef

 11 problems (11 errors, 0 warnings)

Makefile:596: recipe for target 'jslint' failed
make[1]: *** [jslint] Error 1
make[1]: Leaving directory '/home/john/projects/node'
Makefile:115: recipe for target 'test' failed
make: *** [test] Error 2

@eversojk
Copy link
Contributor Author

Ready for review

@Trott
Copy link
Member

Trott commented Mar 22, 2016

Regarding the benchmark tests failing lint, not your problem, it's #5359. I'll put together a fix now unless someone beats me to it.

@Trott
Copy link
Member

Trott commented Mar 22, 2016

The fix has landed, so if you do a rebase against master, it should clear up the linting issue you had.

@Trott
Copy link
Member

Trott commented Mar 22, 2016

OK, so looking at the change you made here, it's all correct, but I think it would be better to format the changes as if the pathObject were the output of path.parse() (just mangled appropriately by removing properties as needed for each example).

So, for example, the value of root will always be / and the value of dir will never have a trailing slash.

> path.parse('/home/user/dir/file.txt')
{ root: '/',
  dir: '/home/user/dir',
  base: 'file.txt',
  ext: '.txt',
  name: 'file' }
> 

So maybe start with that output above and just delete properties as needed (and update your results, of course, as needed)?

Is that clear? I hope I'm making sense...

@jasnell jasnell added the doc Issues and PRs related to the documentations. label Mar 22, 2016
@mscdex mscdex added the path Issues and PRs related to the path subsystem. label Mar 22, 2016
@eversojk
Copy link
Contributor Author

Makes perfect sense, I'll add in those changes.

@eversojk eversojk force-pushed the doc-path-format-more-examples branch from d2e55ae to 42eabf7 Compare March 23, 2016 01:29
@eversojk
Copy link
Contributor Author

I've updated with your suggestions. I used /home/user/dir/file.txt for all examples except to trigger the scenario where dir === root which I used /file.txt.

Would it be preferred that I squash all of my commits and explain what my change is in the squashed commit before having you review?

@eversojk eversojk force-pushed the doc-path-format-more-examples branch from 5d4bfaf to a612f3d Compare March 23, 2016 02:36
@Trott
Copy link
Member

Trott commented Mar 23, 2016

Now is probably a good time to squash, yes.

// `root` will be used if `dir` is not specified and `name` + `ext` will be used
// if `base` is not specified
// `root` will be used if `dir` is not specified
// `name` + `ext` will be used if `base` is not specified
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Would it help to put a period at the end of this line and the one above to make it clear that the subsequent line is a new sentence?

@Trott
Copy link
Member

Trott commented Mar 23, 2016

LGTM with nits that can be ignored if you disagree with them.

@eversojk
Copy link
Contributor Author

I agree with the nits, I debated doing that.

@eversojk eversojk force-pushed the doc-path-format-more-examples branch from ba60503 to 4b0d410 Compare March 23, 2016 03:14
@eversojk
Copy link
Contributor Author

Nits updated and commits squashed.

@Trott
Copy link
Member

Trott commented Mar 23, 2016

/cc @nodejs/documentation

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

LGTM

@Trott
Copy link
Member

Trott commented Mar 23, 2016

LGTM

Nit: Might as well add periods to the end of all the comment sentences and capitalize the first word (where it's not the name of a property!) for consistency. (And if not, then oh well, it will make a nice good first commit for someone else.)

@nwoltman
Copy link
Contributor

@eversojk Since you're already editing this file, would you mind fixing the typo on line 98 (fo -> of)?

This change was to add upon the algorithm description of path.format
by adding examples for unix systems that clarified behavior in
various scenarios.
@eversojk eversojk force-pushed the doc-path-format-more-examples branch from acfee9a to 1cea17f Compare March 24, 2016 00:45
@eversojk
Copy link
Contributor Author

Sentences marked and typo fixed!

@r-52
Copy link
Contributor

r-52 commented Mar 24, 2016

LGTM

jasnell pushed a commit that referenced this pull request Apr 9, 2016
This change was to add upon the algorithm description of path.format
by adding examples for unix systems that clarified behavior in
various scenarios.

PR-URL: #5838
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 9, 2016

Landed in 820844d

@jasnell jasnell closed this Apr 9, 2016
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
This change was to add upon the algorithm description of path.format
by adding examples for unix systems that clarified behavior in
various scenarios.

PR-URL: #5838
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
This change was to add upon the algorithm description of path.format
by adding examples for unix systems that clarified behavior in
various scenarios.

PR-URL: #5838
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
This change was to add upon the algorithm description of path.format
by adding examples for unix systems that clarified behavior in
various scenarios.

PR-URL: #5838
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
This was referenced Apr 21, 2016
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
This change was to add upon the algorithm description of path.format
by adding examples for unix systems that clarified behavior in
various scenarios.

PR-URL: #5838
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
This change was to add upon the algorithm description of path.format
by adding examples for unix systems that clarified behavior in
various scenarios.

PR-URL: #5838
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

path, doc: More examples for path.format()
7 participants