Skip to content

doc: fix invalid path doc comments - #5670

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:mud
Closed

doc: fix invalid path doc comments#5670
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:mud

Conversation

@Trott

Copy link
Copy Markdown
Member

Affected core subsystem(s)

doc, path

Description of change

The format of certain code comments in the path documentation results
in the code blocks being invalid. I also find it confusing at least as
formatted on the website. This change is intended to improve those
comments.

@TrottTrott added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. lts-watch-v4.x labels Mar 12, 2016
@evanlucas

Copy link
Copy Markdown
Contributor

I think the goal was to get syntax highlighting for what was returned. I find it more readable the way that it currently is...

@Trott

Copy link
Copy Markdown
MemberAuthor

/cc @nodejs/documentation

@benjamingr

Copy link
Copy Markdown
Member

LGTM although I'm +0 on this, not sure this makes things better.

@phillipj

Copy link
Copy Markdown
Member

I would go for consistency throughout the docs over personal preference here. ATM it seems we mix two styles:

In buffer.markdown and http.markdown
// returns: true
In path.markdown and querystring.markdown
// returnstrue

I'm +1 on landing this if we do the same change for querystring - might be a separate PR of course. Otherwise buffer and http should be changed.

And multiline results should be commented with // rather than /* .. */ as the former is used for documenting multiline console output in most cases.

@jasnell

Copy link
Copy Markdown
Member

LGTM

@TrottTrott mentioned this pull request Mar 14, 2016
4 tasks
@mscdex

Copy link
Copy Markdown
Contributor

LGTM

The format of certain code comments in the `path` documentation results
in the code blocks being invalid. I also find it confusing at least as
formatted on the website. This change is intended to improve those
comments.
@Trott

Copy link
Copy Markdown
MemberAuthor

And multiline results should be commented with // rather than /* .. */ as the former is used for documenting multiline console output in most cases.

Rebased, made that change, force pushed.

@jasnell

Copy link
Copy Markdown
Member

Still LGTM

@Trott

Copy link
Copy Markdown
MemberAuthor

There are some LGTM's but also at least one (mild?) preference for the current formatting. I see no strong objections. I'll land this in 24 hours or so unless either a collaborator indicates they'd strongly prefer this not land, or unless there is a significant pile-up of milder objections from collaborators (let's say two more people).

Trott added a commit to Trott/io.js that referenced this pull request Mar 17, 2016
The format of certain code comments in the `path` documentation results
in the code blocks being invalid. I also find it confusing at least as
formatted on the website. This change is intended to improve those
comments.
PR-URL: nodejs#5670
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott

Copy link
Copy Markdown
MemberAuthor

Landed in 9eb0ef4

@TrottTrott closed this Mar 17, 2016
@MylesBorins

Copy link
Copy Markdown
Contributor

@Trott this is going to need some manual backporting love

@Trott

Copy link
Copy Markdown
MemberAuthor

@thealphanerd Sure: #5797

@Fishrock123Fishrock123 mentioned this pull request Mar 22, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
The format of certain code comments in the `path` documentation results
in the code blocks being invalid. I also find it confusing at least as
formatted on the website. This change is intended to improve those
comments.
PR-URL: #5670
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Conflicts:
doc/api/path.markdown
@Trott
Trott deleted the mud branch January 13, 2022 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.pathIssues and PRs related to the path subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@Trott@evanlucas@benjamingr@phillipj@jasnell@mscdex@MylesBorins