doc: gh markdown style code blocks #4733

eljefedelrodeodeljefe · 2016-01-17T19:14:08Z

As per discussion on #4726

This changes the code blocks from indentation to ``` for better syntax highlighting.

On the fly changes for typos and highlight breaking markdown have been made. Amongst others this includes:

escaping of asterisks
escaping of underscores e.g. __dirname
various typos
wrong brackets, wrong quotes, forgotten brackets, forgotten quotes
existing code block starts of javascript to js

Improved:
JSON / object notation wasn't consistent. Replaced most with:

{
  key: 'value'
}

Known issues:

not every JSON / object notation has been improved. Should
make another run for this.
some example functions break highlighting due to various combinations of brackets. However changing them means leaving the code style.

~~I would regard 9df0ba4 as optional. It would be a little nicer to have .md though. The choice of extension is from somewhen in 2010, when I see this right, here.~~

doc: renamed doc files to .md, adapted build scripts
This is for consistency and a little less noise for the eye
as in vm.markdown (2 against 8 chars) moved to second PR

rvagg · 2016-01-18T02:55:53Z

re the bash code block, personally I'd like to see bash changed to text or removed. I did that for the README a while back. The code being displayed in most of the blocks labelled as bash is not Bash code, it just happens to include some commandline usage and the syntax highlighting is usually picking up pieces which aren't syntax. In this case the $ isn't Bash, it's just a prompt character.

Regarding renaming, I don't think you're going to get any support on this, it's unnecessary churn that makes the history too hard to follow. In general churn without strong justification gets a -1 around here for this reason.

Regarding unindending code blocks, I'm +1 on that, I've never liked the indent code block formatting and being explicit about js is helpful. This should also help if we go ahead with something along these lines for showing ES5/ESnext.

We need @nodejs/documentation in this discussion too though.

@eljefedelrodeodeljefe in case you weren't aware, there's an active call for involvement in the proposed Docs WG that might interest you: nodejs/docs#2

Qard · 2016-01-18T03:43:07Z

doc/api/child_process.md

+      'pipe', // Pipe child's stdout to parent
+      fs.openSync('err.out', 'w') // Direct child's stderr to a file
+    ]
+});


Since there's churn here anyway, maybe make the indentation consistent while you're at it?

Qard · 2016-01-18T04:20:21Z

There's a few spots missing the js tag.

I'm overall +0 on this. Any other docs folks have thought on it?

bengl · 2016-01-18T04:38:45Z

From a @nodejs/documentation perspective, with respect to de-indenting and using language-aware code fences, this change is consistent with our new style guide (see the bottom).

While renaming from .markdown to .md is also consistent with with our style guide, it's a huge amount of churn on the files, as @rvagg already mentioned.

stevemao · 2016-01-18T10:04:17Z

The code being displayed in most of the blocks labelled as bash is not Bash code, it just happens to include some commandline usage and the syntax highlighting is usually picking up pieces which aren't syntax.

This bug should be fixed.

eljefedelrodeodeljefe · 2016-01-18T10:48:33Z

Added @Qard's remarks in be6d6e8. @rvagg I agree on the churn. However maybe this will be the only time this can be touched. So please ignore the commit, while rebasing if no-one is in favor. @stevemao good link. I was looking at it and it seems to render okay, @rvagg . I have no strong opinion on bash. However you like it.

@rvagg thanks for welcoming me on the other PR and pointing to the docs wg. It's fun so far. :)

eljefedelrodeodeljefe · 2016-01-18T10:53:21Z

It's really unfortunate that git mv doesn't save history visibly. Or that GitHub isn't dragging git log --follow doc/api/stream.md data into history.

rvagg · 2016-01-18T10:55:01Z

re bash, in this case it looks OK I guess, it's technically incorrect but mostly works, since it's already there don't bother changing it I suppose!

rvagg · 2016-01-18T10:57:12Z

oh, and regarding the rename of the files, it's making the commit diff really large and hard to review so you may get more traction towards landing if you clean it up by removing that commit (let us know if you need any hints on the git-fu for that).

eljefedelrodeodeljefe · 2016-01-18T11:00:58Z

I will try something like this gist later https://gist.github.com/emiller/6769886 But this is probably too hard, since it's likely rewriting the history (into the new files). I will see. Thanks for the help!

stevemao · 2016-01-18T11:01:55Z

Hi @rvagg Could you link git-fu here?

rvagg · 2016-01-18T11:10:05Z

@stevemao sorry, just being colloquial, I was just meaning advanced git usage. In this case to remove a commit I'd use git rebase -i HEAD~4 and you can just remove a commit right out of the list. The problem is going to be that the later commit(s) depend so much on the file moves and I'm not sure git will be smart enough to know to edit the unmoved files. Then to push the edited branch back up you git push ... --force and it'll end up in here for us to see in its new form.

stevemao · 2016-01-18T11:13:52Z

Oh yeah true... Thanks for the clarification.

Fishrock123 · 2016-01-18T17:55:18Z

@eljefedelrodeodeljefe Could you please move the .markdown -> .md rename to a separate PR? Thanks!

eljefedelrodeodeljefe · 2016-01-18T18:32:57Z

@Fishrock123 removed the commit by rebase and force pushing. Think the other PR got a little whacked.

I tell you....after that git-fu I appreciate your work even more :) cc @rvagg

Fishrock123 · 2016-01-18T18:51:14Z

doc/api/repl.markdown

-    2
-    3
+```bash
+mjr:~$ node


Note for this PR or the future, should probably remove mjr:~

silverwind · 2016-01-18T19:50:35Z

More comparision of bash vs console highlighting:

bash

$ NODE_DEBUG=cluster node server.js
23521,Master Worker 23524 online
23521,Master Worker 23526 online
23521,Master Worker 23523 online
23521,Master Worker 23528 online

console

$ NODE_DEBUG=cluster node server.js
23521,Master Worker 23524 online
23521,Master Worker 23526 online
23521,Master Worker 23523 online
23521,Master Worker 23528 online

bash

$ node script.js 2> error.log | tee info.log

console

$ node script.js 2> error.log | tee info.log

bash

% node debug myscript.js
< debugger listening on port 5858
connecting... ok
break in /home/indutny/Code/git/indutny/myscript.js:1
  1 x = 5;
  2 setTimeout(function () {
  3   debugger;
debug>

console

% node debug myscript.js
< debugger listening on port 5858
connecting... ok
break in /home/indutny/Code/git/indutny/myscript.js:1
  1 x = 5;
  2 setTimeout(function () {
  3   debugger;
debug>

bash

$ node debug myscript.js
< debugger listening on port 5858
connecting... ok
break in /home/indutny/Code/git/indutny/myscript.js:1
  1 x = 5;
  2 setTimeout(function () {
  3   debugger;
debug> cont
< hello
break in /home/indutny/Code/git/indutny/myscript.js:3
  1 x = 5;
  2 setTimeout(function () {
  3   debugger;
  4   console.log('world');
  5 }, 1000);
debug> next
break in /home/indutny/Code/git/indutny/myscript.js:4
  2 setTimeout(function () {
  3   debugger;
  4   console.log('world');
  5 }, 1000);
  6 console.log('hello');
debug> repl
Press Ctrl + C to leave debug repl
> x
5
> 2+2
4
debug> next
< world
break in /home/indutny/Code/git/indutny/myscript.js:5
  3   debugger;
  4   console.log('world');
  5 }, 1000);
  6 console.log('hello');
  7
debug> quit
$

console

$ node debug myscript.js
< debugger listening on port 5858
connecting... ok
break in /home/indutny/Code/git/indutny/myscript.js:1
  1 x = 5;
  2 setTimeout(function () {
  3   debugger;
debug> cont
< hello
break in /home/indutny/Code/git/indutny/myscript.js:3
  1 x = 5;
  2 setTimeout(function () {
  3   debugger;
  4   console.log('world');
  5 }, 1000);
debug> next
break in /home/indutny/Code/git/indutny/myscript.js:4
  2 setTimeout(function () {
  3   debugger;
  4   console.log('world');
  5 }, 1000);
  6 console.log('hello');
debug> repl
Press Ctrl + C to leave debug repl
> x
5
> 2+2
4
debug> next
< world
break in /home/indutny/Code/git/indutny/myscript.js:5
  3   debugger;
  4   console.log('world');
  5 }, 1000);
  6 console.log('hello');
  7
debug> quit
$

I'd say we use plain ````` fences. None of bash, `console` or even `js` work well enough for REPL/debugger.

Qard · 2016-01-18T23:39:09Z

Your effort in this PR is greatly appreciated, however I think it's currently too big to properly review.

I think if this was split up into a separate PR for each markdown file, only changing the indentation and fencing, I would be good to merge it. The other miscellaneous things should also be split out to separate PRs.

silverwind · 2016-01-19T00:17:13Z

separate PR for each markdown file

For the purpose of viewing all changes on GitHub, it should suffice to do one commit per file. And yeah, let's focus on getting the fencing done first and do additional changes that have been suggested in seperate PRs.

Qard · 2016-01-19T00:27:11Z

You could split to a commit for each file, but that still requires that each file is validated entirely before anything in the PR can be merged. By splitting it to separate PRs, more easily consumable chunks can be validated and merged in isolation. Either way works, but I feel like it might go smoother if there are separate PRs.

silverwind · 2016-01-19T00:47:11Z

To keep things simple, I suggest eliminate "extra" changes (typo fixes) except the fencing from the current diff. We can then review it locally (curl -L https://github.com/nodejs/node/pull/4733 | git show).

eljefedelrodeodeljefe · 2016-01-19T09:37:58Z

Okay. Will rebase, and make a third PR for it :) In the initial commit though, there are some things I corrected immediately because they were breaking markdown rendering on GitHub and Editors. I'd like to leave them in.

I stand corrected. Checking out from master left me with lots of merge conflicts. Could we agree on landing this by rebasing the commits you can easily apply and afterwards let me cherry-pick the changes?

@Qard, @silverwind I know it's churn. But the idea was to have churn in just one PR instead of three. Rod was right though, that especially renaming needs consensus and therefore needs to be separated. Anyhow the first commit (just plain code blocks) is probably as hard to review as the whole PR with all the changes. :)

This changes the code blocks from 4-space indentation to ``` fences for better syntax highlighting and future linting support. Minor On-the-fly changes for typos and highlight breaking markdown have been made. JSON-Style objects have been changed so their closing bracket is on the same line as the opening one. Known issues: * Not every JSON / object notation has been improved. Should make another run for this. * Some example functions break hightlighting due to various combinations of brackets. However changing them means leaving the code style. Fixes: #4726 PR-URL: #4733 Reviewed-By: Roman Reiss <me@silverwind.io>

silverwind · 2016-01-21T19:57:37Z

Thanks! Landed in e436272. Feel free to follow up with more changes that were discussed here.

This changes the code blocks from 4-space indentation to ``` fences for better syntax highlighting and future linting support. Minor On-the-fly changes for typos and highlight breaking markdown have been made. JSON-Style objects have been changed so their closing bracket is on the same line as the opening one. Known issues: * Not every JSON / object notation has been improved. Should make another run for this. * Some example functions break hightlighting due to various combinations of brackets. However changing them means leaving the code style. Fixes: #4726 PR-URL: #4733 Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins · 2016-01-28T21:55:00Z

There are a number of changes to docs that have not yet landed that need to be back ported. Once those land this commit will likely have to be manually backported to LTS

stevemao · 2016-02-10T10:48:11Z

Aside: (don't wanna make this sound like an advertisement but it's actually useful)
There are PRs recently containing a lot of changes. If you ever have problem reviewing the change, I recommend you guys trying https://github.com/stevemao/diff-so-fancy

eljefedelrodeodeljefe · 2016-02-10T12:29:52Z

@stevemao @silverwind had something similar with HTML output. In either form it's superuseful, thanks.

stevemao · 2016-02-10T12:34:01Z

I tried it. Another good tool :)

This changes the code blocks from 4-space indentation to ``` fences for better syntax highlighting and future linting support. Minor On-the-fly changes for typos and highlight breaking markdown have been made. JSON-Style objects have been changed so their closing bracket is on the same line as the opening one. Known issues: * Not every JSON / object notation has been improved. Should make another run for this. * Some example functions break hightlighting due to various combinations of brackets. However changing them means leaving the code style. Fixes: #4726 PR-URL: #4733 Reviewed-By: Roman Reiss <me@silverwind.io>

This changes the code blocks from 4-space indentation to ``` fences for better syntax highlighting and future linting support. Minor On-the-fly changes for typos and highlight breaking markdown have been made. JSON-Style objects have been changed so their closing bracket is on the same line as the opening one. Known issues: * Not every JSON / object notation has been improved. Should make another run for this. * Some example functions break hightlighting due to various combinations of brackets. However changing them means leaving the code style. Fixes: nodejs#4726 PR-URL: nodejs#4733 Reviewed-By: Roman Reiss <me@silverwind.io>

These are likely left over from backporting nodejs#4733.

These are likely left over from backporting #4733. PR-URL: #7681 Reviewed-By: Myles Borins <myles.borins@gmail.com>

r-52 added the doc Issues and PRs related to the documentations. label Jan 17, 2016

Qard reviewed Jan 18, 2016
View reviewed changes

eljefedelrodeodeljefe mentioned this pull request Jan 18, 2016

doc: renamed doc files to .md, adapted build scripts #4747

Closed

Fishrock123 reviewed Jan 18, 2016
View reviewed changes

doc/api/repl.markdown

2

3

```bash

mjr:~$ node

Copy link

Contributor

Fishrock123 Jan 18, 2016

Choose a reason for hiding this comment

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

Note for this PR or the future, should probably remove mjr:~

silverwind closed this Jan 21, 2016

eljefedelrodeodeljefe deleted the feature/doc-gh-markdown-style-code-blocks branch January 21, 2016 20:51

silverwind added the lts-watch-v4.x label Jan 21, 2016

silverwind removed their assignment Jan 21, 2016

This was referenced Jan 21, 2016

doc: proper markdown escaping -> \_\_, \*, \_ #4805

Closed

doc: harmonize $ node command line notation #4806

Closed

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v5.5.1 (Stable) #5141

Closed

a0viedo mentioned this pull request Feb 8, 2016

meta: style guide nodejs/docs#80

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Feb 22, 2016

addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016

doc: remove superfluos backticks in process.md

d8d374c

These are likely left over from backporting nodejs#4733.

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016

doc: remove superfluos backticks in process.md

4d96bba

These are likely left over from backporting #4733. PR-URL: #7681 Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins pushed a commit that referenced this pull request Jul 12, 2016

doc: remove superfluos backticks in process.md

bf52c6b

These are likely left over from backporting #4733. PR-URL: #7681 Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins pushed a commit that referenced this pull request Jul 14, 2016

doc: remove superfluos backticks in process.md

21b1285

These are likely left over from backporting #4733. PR-URL: #7681 Reviewed-By: Myles Borins <myles.borins@gmail.com>

MylesBorins pushed a commit that referenced this pull request Jul 14, 2016

doc: remove superfluos backticks in process.md

1a5c025

These are likely left over from backporting #4733. PR-URL: #7681 Reviewed-By: Myles Borins <myles.borins@gmail.com>

This was referenced Oct 16, 2021

[Snyk] Security upgrade mocha from 7.2.0 to 9.1.2 baby636/node#32

Open

[Snyk] Security upgrade mocha from 7.2.0 to 9.1.2 ryan-ally/node#49

Open

[Snyk] Security upgrade mocha from 7.2.0 to 9.1.2 XirdigH/node-22#69

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: gh markdown style code blocks #4733

doc: gh markdown style code blocks #4733

eljefedelrodeodeljefe commented Jan 17, 2016

rvagg commented Jan 18, 2016

Qard Jan 18, 2016

Qard commented Jan 18, 2016

bengl commented Jan 18, 2016

stevemao commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

rvagg commented Jan 18, 2016

rvagg commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

stevemao commented Jan 18, 2016

rvagg commented Jan 18, 2016

stevemao commented Jan 18, 2016

Fishrock123 commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

Fishrock123 Jan 18, 2016

silverwind commented Jan 18, 2016

Qard commented Jan 18, 2016

silverwind commented Jan 19, 2016

Qard commented Jan 19, 2016

silverwind commented Jan 19, 2016

eljefedelrodeodeljefe commented Jan 19, 2016

silverwind commented Jan 21, 2016

MylesBorins commented Jan 28, 2016

stevemao commented Feb 10, 2016

eljefedelrodeodeljefe commented Feb 10, 2016

stevemao commented Feb 10, 2016

doc: gh markdown style code blocks #4733

doc: gh markdown style code blocks #4733

Conversation

eljefedelrodeodeljefe commented Jan 17, 2016

rvagg commented Jan 18, 2016

Qard Jan 18, 2016

Choose a reason for hiding this comment

Qard commented Jan 18, 2016

bengl commented Jan 18, 2016

stevemao commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

rvagg commented Jan 18, 2016

rvagg commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

stevemao commented Jan 18, 2016

rvagg commented Jan 18, 2016

stevemao commented Jan 18, 2016

Fishrock123 commented Jan 18, 2016

eljefedelrodeodeljefe commented Jan 18, 2016

Fishrock123 Jan 18, 2016

Choose a reason for hiding this comment

silverwind commented Jan 18, 2016

bash

console

bash

console

bash

console

bash

console

Qard commented Jan 18, 2016

silverwind commented Jan 19, 2016

Qard commented Jan 19, 2016

silverwind commented Jan 19, 2016

eljefedelrodeodeljefe commented Jan 19, 2016

silverwind commented Jan 21, 2016

MylesBorins commented Jan 28, 2016

stevemao commented Feb 10, 2016

eljefedelrodeodeljefe commented Feb 10, 2016

stevemao commented Feb 10, 2016