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.

Sign up for GitHub

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

Jump to bottom

doc: explain path.format expected properties #5801

Closed
wants to merge 1 commit into from
Closed

doc: explain path.format expected properties #5801

wants to merge 1 commit into from

Conversation

eversojk
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • 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]?
  • [doesn't fix a bug] 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

explain path.format expected properties

fixes #5746

@eversojk eversojk mentioned this pull request Mar 19, 2016
Closed
@mscdex mscdex added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Mar 19, 2016
@benjamingr
Copy link
Member

Thanks, please wrap lines at 80 characters if that's not too much work. It could also be be nice to expand a little here.

@eversojk
Copy link
Contributor Author

I'll fix the long line, thanks for pointing that out. I was going to expand further with more examples in a different PR fixing #5747. Would adding more examples in a separate PR be preferred?

@benjamingr
Copy link
Member

I think one PR is fine but @Trott is more experienced here than I.

@eversojk eversojk force-pushed the doc-path-expected-properties branch from d8fde07 to 8c469c2 Compare March 19, 2016 17:52
@eversojk
Copy link
Contributor Author

Ok, I'll wait until he chimes in before adding the additional examples.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

One PR is fine as long as the changes are all relevant to the same issue. There can be multiple commits if necessary.

@Trott
Copy link
Member

Trott commented Mar 21, 2016

One PR is fine in this case, but two is also fine, and since you already have it as two, maybe leave it as that. As this is your first commit (right?), it might be best to keep it small anyway so we can focus on process and not endless nits on grammar and style.

@Trott
Copy link
Member

Trott commented Mar 21, 2016

LGTM. If you can squash the two commits down to one and force push, that would be great. If not, whoever lands this can do it.

Trott
Trott reviewed Mar 21, 2016
View reviewed changes
doc/api/path.markdown
If `pathObject` has all expected properties, the returned string will be a
concatenation of the `dir` property, the platform-dependent path separator, and
the `base` property.
If `pathObject` has the `dir` and `base` properties, the returned string will
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Might be better as "has dir and base properties" (no "the").

@eversojk
doc: explain path.format expected properties
c682329 
doc: limit path.format line to 80 characters

doc: path.format remove the
@eversojk eversojk force-pushed the doc-path-expected-properties branch from b680849 to c682329 Compare March 22, 2016 01:10
@eversojk
Copy link
Contributor Author

@Trott I've added your suggestion and squashed my commits into one and should be good to go. I'll add the extra examples on #5747.

@Trott
Copy link
Member

Trott commented Mar 22, 2016

Still LGTM.

Nit: The body of the commit message should probably be a sentence explaining the change rather than the first line of the commits that we're squashed.

benjamingr pushed a commit that referenced this pull request Mar 22, 2016
@eversojk @benjamingr
doc: explain path.format expected properties
d16d904 
Explain the expected properties in path.format

Fixes: #5746
PR-URL: #5801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@benjamingr
Copy link
Member

Thanks for the contribution @eversojk ! Landed in d16d904

@benjamingr benjamingr closed this Mar 22, 2016
@eversojk
Copy link
Contributor Author

@benjamingr Thanks! Also thanks for your input @jasnell @Trott

@eversojk eversojk deleted the doc-path-expected-properties branch March 23, 2016 01:19
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
@eversojk @evanlucas
doc: explain path.format expected properties
93638e1 
Explain the expected properties in path.format

Fixes: #5746
PR-URL: #5801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@eversojk
doc: explain path.format expected properties
c2514b5 
Explain the expected properties in path.format

Fixes: #5746
PR-URL: #5801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@eversojk
doc: explain path.format expected properties
79d26ae 
Explain the expected properties in path.format

Fixes: #5746
PR-URL: #5801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@eversojk @evanlucas
doc: explain path.format expected properties
5676a35 
Explain the expected properties in path.format

Fixes: #5746
PR-URL: #5801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

path, doc: Explain "expected properties"
6 participants
@eversojk @benjamingr @jasnell @Trott @mscdex @MylesBorins