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: use blockquotes for Stability: markers #7757

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

@addaleax addaleax added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jul 15, 2016
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/4130/

/cc @ChALkeR @nodejs/documentation


> Stability: 3 - Locked
> Only fixes related to security, performance, or bug fixes will be accepted.
> Please do not suggest API changes in this area; they will be refused.
Copy link
Member

@ChALkeR ChALkeR Jul 16, 2016

Choose a reason for hiding this comment

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

This block doesn't look good on GitHub:

  1. This is interpreted as one blockquote, not four.
  2. Line breaks don't work.

Adding a manual line break there to fix how it looks on GitHub confuses the doc tool and it treats those as separated blocks, with only the first line being colored.

@ChALkeR
Copy link
Member

ChALkeR commented Jul 16, 2016

Also, this change added extra margin to the stability markers in the html produced by the doc tool by wrapping these <pre> blocks in a blockquote. Can we somehow get rid of the blockquote or change its style?

As for the documentation.md changes — perhaps it would make sense to support both ways (code block and blockquote) and use the code block only in the documentation.md file?

@ChALkeR
Copy link
Member

ChALkeR commented Jul 16, 2016

Nice, thanks!

Just noticed: not sure why, but I have an "undefined" Text node under each stability level blockquote in the generated HTML =). That doesn't affect the stability code blocks in documentation.md.

@addaleax
Copy link
Member Author

@ChALkeR Thanks for noticing! Should be fixed now.

@jasnell
Copy link
Member

jasnell commented Jul 20, 2016

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented Jul 20, 2016

@addaleax Something looks strange in the json output to me.

{
  "textRaw": "fs.existsSync(path)",
  "type": "method",
  "name": "existsSync",
  "meta": {
    "added": [
      "v0.1.21"
    ],
    "deprecated": [
      "v1.0.0"
    ]
  },
  "desc": "<p>Stability: 0 - Deprecated: Use [<code>fs.statSync()</code>][] or [<code>fs.accessSync()</code>][]\ninstead.</p>\n<p>Synchronous version of [<code>fs.exists()</code>][].\nReturns <code>true</code> if the file exists, <code>false</code> otherwise.</p>\n",
  "signatures": [
    {
      "params": [
        {
          "textRaw": "`path` {String | Buffer} ",
          "name": "path",
          "type": "String | Buffer"
        }
      ]
    },
    {
      "params": [
        {
          "name": "path"
        }
      ]
    }
  ]
},

Why are there two signatures there now?

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Jul 20, 2016
@addaleax
Copy link
Member Author

Yeah, I was planning on double-checking the JSON output anyway, sorry.

@@ -436,7 +436,7 @@ without exiting the domain.

### domain.dispose()

Stability: 0 - Deprecated. Please recover from failed IO actions
> Stability: 0 - Deprecated. Please recover from failed IO actions
Copy link
Member

Choose a reason for hiding this comment

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

The next line also should be converted to > or at least the padding should be removed. Else, this is how it renders:
screenshot_20160720_200119

@ChALkeR
Copy link
Member

ChALkeR commented Jul 20, 2016

Note: this also changes JSON output from being wrapped into <pre><code> into being wrapped into <p>. I'm not saying that it's a bad thing — I don't know =).

Also, it now generates things like [<code>fs.accessSync()</code>][] instead of [fs.accessSync()][] (in the JSON). Btw, I'm not sure if [fs.accessSync()][] is correct there, shouldn't it be a link, as everything looks html-ish there? I'm not familiar with the json output.

@ChALkeR
Copy link
Member

ChALkeR commented Jul 20, 2016

Markdown — LGTM with one comment in domain.md.
HTML — LGTM.
JSON — some comments above, but I'm not sure about most of those myself, so it shouldn't block this.

@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label Jul 25, 2016
@addaleax
Copy link
Member Author

Rebased, and fixed the docs nit.

I’ll have to look into the JSON output myself, that one’s confusing… you’re right, there’s a difference here, but it looks like this change actually resulted in prepending the correct signature information where it was previously left out.

@ChALkeR
Copy link
Member

ChALkeR commented Jul 25, 2016

HTML output and *.md changes — LGTM
JSON — looks reasonable, but again — I'm not familiar with json output.

/cc @nodejs/documentation again, @eljefedelrodeodeljefe, @silverwind, @firedfox.

Actual output changes here — https://gist.github.com/ChALkeR/9fc933caa0faca63ba5105b9bc453ec2.
HTML change (first file) is fine, that's just due to #7757 (comment) bugfix.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 3, 2016

/cc @nodejs/collaborators

@targos
Copy link
Member

targos commented Aug 3, 2016

LGTM

@silverwind
Copy link
Contributor

Got some screenshots of the docs before/after? (sorry for being so lazy)

@ChALkeR
Copy link
Member

ChALkeR commented Aug 3, 2016

@silverwind There are no actual changes in the generated html except for a bugfix of https://nodejs.org/api/repl.html#repl_node_repl_history_file, see the diff linked at #7757 (comment) — so no other visual changes there.

GitHub render changes could be viewed by comparing https://github.com/nodejs/node/blob/master/doc/api/tls.md#deprecated-apis with https://github.com/addaleax/node/blob/doc-blockquote-stability/doc/api/tls.md#deprecated-apis, for example.

Screenshotting JSON output is pretty pointless. 😉

@addaleax
Copy link
Member Author

addaleax commented Aug 3, 2016

Yeah, screenshots don’t really make sense here, and apart from that @ChALkeR’s diff is probably the best one can get for the JSON output. That one does look okay to me.

@silverwind
Copy link
Contributor

silverwind commented Aug 3, 2016

Thanks. JSON looks more correct than before at least. LGTM.

Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (nodejs#7729).
@addaleax
Copy link
Member Author

addaleax commented Aug 3, 2016

Rebased to resolve conflict with 29e49fc, new CI: https://ci.nodejs.org/job/node-test-commit/4393/

I’d like to land this if it’s green.

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

This still LGTM. Is this ready to go?

@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

Yes, I’ll land it now (thanks for the ping!)

addaleax added a commit that referenced this pull request Aug 4, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (#7729).

PR-URL: #7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

Landed in c809b88, thanks for the reviews everyone!

@addaleax addaleax closed this Aug 4, 2016
@addaleax addaleax deleted the doc-blockquote-stability branch August 4, 2016 21:31
ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Aug 7, 2016
Specifies the configuration for remark-lint, a markdown linter.

This configuration does not cause any warnings on any of the currently
present *.md files (ignoring thirdparty).

It is useful not only for possible future tooling that would check the
markdown files syntax, but also as a configuration for editor plugins,
e.g. linter-markdown for atom-linter.

Refs: nodejs#7637
Refs: nodejs#7727
Refs: nodejs#7757
PR-URL: nodejs#7729
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (#7729).

PR-URL: #7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>

Conflicts:
	doc/api/punycode.md
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Specifies the configuration for remark-lint, a markdown linter.

This configuration does not cause any warnings on any of the currently
present *.md files (ignoring thirdparty).

It is useful not only for possible future tooling that would check the
markdown files syntax, but also as a configuration for editor plugins,
e.g. linter-markdown for atom-linter.

Refs: #7637
Refs: #7727
Refs: #7757
PR-URL: #7729
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
MylesBorins pushed a commit that referenced this pull request Sep 9, 2016
Specifies the configuration for remark-lint, a markdown linter.

This configuration does not cause any warnings on any of the currently
present *.md files (ignoring thirdparty).

It is useful not only for possible future tooling that would check the
markdown files syntax, but also as a configuration for editor plugins,
e.g. linter-markdown for atom-linter.

Refs: #7637
Refs: #7727
Refs: #7757
PR-URL: #7729
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
Specifies the configuration for remark-lint, a markdown linter.

This configuration does not cause any warnings on any of the currently
present *.md files (ignoring thirdparty).

It is useful not only for possible future tooling that would check the
markdown files syntax, but also as a configuration for editor plugins,
e.g. linter-markdown for atom-linter.

Refs: #7637
Refs: #7727
Refs: #7757
PR-URL: #7729
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
@MylesBorins
Copy link
Contributor

@addaleax this is not landing cleanly, likely due to a bunch of other commits needing to be backported. please feel free to manually backport

addaleax added a commit to addaleax/node that referenced this pull request Sep 30, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (nodejs#7729).

PR-URL: nodejs#7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (#7729).

PR-URL: #7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (#7729).

PR-URL: #7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (#7729).

PR-URL: #7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Specifies the configuration for remark-lint, a markdown linter.

This configuration does not cause any warnings on any of the currently
present *.md files (ignoring thirdparty).

It is useful not only for possible future tooling that would check the
markdown files syntax, but also as a configuration for editor plugins,
e.g. linter-markdown for atom-linter.

Refs: #7637
Refs: #7727
Refs: #7757
PR-URL: #7729
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (#7729).

PR-URL: #7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Specifies the configuration for remark-lint, a markdown linter.

This configuration does not cause any warnings on any of the currently
present *.md files (ignoring thirdparty).

It is useful not only for possible future tooling that would check the
markdown files syntax, but also as a configuration for editor plugins,
e.g. linter-markdown for atom-linter.

Refs: #7637
Refs: #7727
Refs: #7757
PR-URL: #7729
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Use blockquotes instead of code blocks for stability markers in
the docs. Doing that:

- Makes the makers appear correctly when viewed e.g. on github.
- Allows remark-lint rules like `no-undefined-references` to work
  properly (#7729).

PR-URL: #7757
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants