Skip to content

Conversation

@firedfox
Copy link
Contributor

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

tools

Description of change

Update module tools/doc/node_modules/marked. Customize renderer to remove id from heading.

Diff result for all.html http://106.187.89.52/tmp/diff-683f2a7-doc-html-all.html
Diff result for all.json http://106.187.89.52/tmp/diff-683f2a7-doc-json-all.html

The newly updated marked module fixed several problems in the doc.

Removed unnecessary </p><p>

In addons.md:

Please see the examples below for further information or <https://github.com/arturadib/node-qt> for an example in production.

HTML result from:

<p>Please see the examples below for further information or </p><p><ahref="https://github.com/arturadib/node-qt">https://github.com/arturadib/node-qt</a> for an example in production. </p>

to:

<p>Please see the examples below for further information or <ahref="https://github.com/arturadib/node-qt">https://github.com/arturadib/node-qt</a> for an example in production.</p>
Fixed unordered list

In util.md:

`util.inspect.styles` is a map assigning each style a color from `util.inspect.colors`. Highlighted styles and their default values are: *`number` (yellow) *`boolean` (yellow) *`string` (green) *`date` (magenta) *`regexp` (red) *`null` (bold) *`undefined` (grey) *`special` - only function at this time (cyan) *`name` (intentionally no styling)

HTML result from:

<p><code>util.inspect.styles</code> is a map assigning each style a color from <code>util.inspect.colors</code>. Highlighted styles and their default values are: <em><code>number</code> (yellow) </em><code>boolean</code> (yellow) <em><code>string</code> (green) </em><code>date</code> (magenta) <em><code>regexp</code> (red) </em><code>null</code> (bold) <em><code>undefined</code> (grey) </em><code>special</code> - only function at this time (cyan) * <code>name</code> (intentionally no styling) </p>

to:

<p><code>util.inspect.styles</code> is a map assigning each style a color from <code>util.inspect.colors</code>. Highlighted styles and their default values are:</p><ul><li><code>number</code> (yellow)</li><li><code>boolean</code> (yellow)</li><li><code>string</code> (green)</li><li><code>date</code> (magenta)</li><li><code>regexp</code> (red)</li><li><code>null</code> (bold)</li><li><code>undefined</code> (grey)</li><li><code>special</code> - only function at this time (cyan)</li><li><code>name</code> (intentionally no styling)</li></ul>
Removed redundant new lines

In addons.md:

To test:

HTML result from:

<p>To test: </p>

to:

<p>To test:</p>
code element class name changes

It changed the class name for code element from cpp / js / javascript to lang-cpp / lang-js / lang-javascript. It does not matter because there is no style defined for these class names.

@mscdexmscdex added the tools Issues and PRs related to the tools directory. label Apr 26, 2016
@addaleaxaddaleax added the doc Issues and PRs related to the documentations. label Apr 26, 2016
@silverwind
Copy link
Contributor

@nodejs/documentation

Copy link
Member

Choose a reason for hiding this comment

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

const?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't really fit the style of this file. I guess we shoul to es6ify the doctool later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two lines below this, template strings are used :P

Copy link
ContributorAuthor

@firedfoxfiredfoxMay 5, 2016

Choose a reason for hiding this comment

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

Yes it's a problem. I think I'd better change the template string to string replace (edit: or just string add) in order to keep the style consistent.

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, I also used template strings in #6495, I don’t really think that’s a problem. And sorry for the merge conflict, btw, but that should be limited to the package.json.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It doesn't matter. I rebased upstream and resolved the conflict.
I just changed the template to string add before I saw your latest comment :D
I think either is OK. At least the style is consistent now. We can es6ify it later.

@silverwind
Copy link
Contributor

Let's see if the doctool tests pass:

https://ci.nodejs.org/job/node-test-pull-request/2500/

@firedfoxfiredfoxforce-pushed the update-marked-dependency branch 2 times, most recently from 7343c52 to 062bdf2CompareMay 5, 2016 03:13
@firedfox
Copy link
ContributorAuthor

There seems to be something wrong with the armv8-ubuntu1404 environment.
image

@silverwind
Copy link
Contributor

silverwind commented May 6, 2016

Probably nothing to worry about, I bet it'll be green next run. Here's another run:https://ci.nodejs.org/job/node-test-pull-request/2523/

LGTM, by the way.

@addaleax
Copy link
Member

CI is red, example failure: https://ci.nodejs.org/job/node-test-commit-arm/3159/nodes=armv7-wheezy/tapTestReport/test.tap-1068/

That’s just whitespace changes in the test results, though. It may be nice to do something there like we do for the HTML output tests, i.e. strip whitespaces in both expected and actual output and compare that.

@firedfoxfiredfoxforce-pushed the update-marked-dependency branch 2 times, most recently from fad0eac to d3ceaa8CompareMay 7, 2016 04:55
Update module marked. Customize renderer to remove id from heading.
@firedfoxfiredfoxforce-pushed the update-marked-dependency branch from d3ceaa8 to c3208bcCompareMay 7, 2016 04:59
@firedfox
Copy link
ContributorAuthor

@addaleax It's my bad. I modified the test case to make my local tests pass but forgot to commit it. It's updated now. I just removed extra \ns from the expected data. Those \ns are bugs of the old version marked module. I think they will not change in future. So we don't have to take more effort to strip them from json objects.

@addaleax
Copy link
Member

@addaleax
Copy link
Member

CI is green.

@firedfox the Author: field in your git commit is set to firedfox, but you are listed with your full name in the AUTHORS file. If you want, this can be fixed in when landing the commit?

@firedfox
Copy link
ContributorAuthor

Oh yes, please change the Author field to my full name. Thank you!

@addaleax
Copy link
Member

Landed in 3f69ea5. Thanks!

You can set this via git config [--global] user.name '…', btw. :)

@addaleaxaddaleax closed this May 7, 2016
addaleax pushed a commit that referenced this pull request May 7, 2016
Update module marked. Customize renderer to remove id from heading. PR-URL: #6396 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@bnoordhuis
Copy link
Member

FYI, this PR looks to be breaking tools/doc/addon-verify.js; as a result, the embedded add-ons in the documentation are no longer built and tested.

I'm going to look into what's causing the breakage but if I can't figure it out or it's too much effort to fix, I'll be moving to revert it.

@addaleax
Copy link
Member

@bnoordhuisaddaleax/node@6e4342f seems to do the trick?

@bnoordhuis
Copy link
Member

@addaleax Yes, seems to be working.

@bnoordhuis
Copy link
Member

#6652

@bnoordhuis
Copy link
Member

Release people: if this gets backported, then please also cherry-pick commit 4f925dd from #6652.

evanlucas pushed a commit that referenced this pull request May 17, 2016
Update module marked. Customize renderer to remove id from heading. PR-URL: #6396 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Jul 12, 2016
Update module marked. Customize renderer to remove id from heading. PR-URL: nodejs#6396 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Update module marked. Customize renderer to remove id from heading. PR-URL: #6396 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Update module marked. Customize renderer to remove id from heading. PR-URL: #6396 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Update module marked. Customize renderer to remove id from heading. PR-URL: #6396 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@firedfox@silverwind@addaleax@bnoordhuis@thefourtheye@mscdex@MylesBorins