tools,doc: allow page titles to contain inline code by aduh95 · Pull Request #35003 · nodejs/node

aduh95 · 2020-08-31T23:18:36Z

Previously the HTML title would be cut to the first text node only. This PR concats the text value of all the node children of the first heading.

This is currently visible on Modules: module API page:

Currently, on nodejs.org:

<title>Modules:  | Node.js v14.9.0 Documentation</title>

With this PR:

<title>Modules: module API | Node.js v15.0.0 Documentation</title>

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Previously the HTML title would be cut to the first text node only.

tools/doc/package.json

tools/doc/html.js

richardlau · 2020-09-01T14:07:17Z

If possible could you add a test to https://github.com/nodejs/node/tree/master/test/doctool please? (Maybe to https://github.com/nodejs/node/blob/master/test/doctool/test-doctool-html.js?)

aduh95 · 2020-09-02T09:47:06Z

If possible could you add a test

@richardlau done! I've also make some refactoring to the test code (sample_document.md was parsed twice with the same code, there was an unused async keyword, I've added some more modern syntax to make it more readable, …), I can remove that from the current PR if we feel it doesn't belong here.

nodejs-github-bot · 2020-09-02T10:56:10Z

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

nodejs-github-bot · 2020-09-02T14:13:47Z

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

nodejs-github-bot · 2020-09-02T15:08:40Z

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

github-actions · 2020-09-02T16:48:11Z

Commit Queue failed

- Loading data for nodejs/node/pull/35003
✔  Done loading data for nodejs/node/pull/35003
----------------------------------- PR info ------------------------------------
Title      tools,doc: allow page titles to contain inline code (#35003)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:fix-module-page-title -> nodejs:master
Labels     doc, tools
Commits    3
 - tools,doc: allow page titles to contain inline code
 - v12 compatible
 - Add tests and support for nested elements
Committers 1
 - Antoine du HAMEL 
PR-URL: https://github.com/nodejs/node/pull/35003
Reviewed-By: Richard Lau 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35003
Reviewed-By: Richard Lau 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-09-02T15:08:40Z: https://ci.nodejs.org/job/node-test-pull-request/33014/
- Querying data for job/node-test-pull-request/33014/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Mon, 31 Aug 2020 23:18:36 GMT
   ✔  Approvals: 2
   ✔  - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/35003#pullrequestreview-480703930
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35003#pullrequestreview-480855115
   ✖  This PR needs to wait 6 more hours to land
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

github-actions · 2020-09-02T23:09:17Z

Commit Queue failed

- Loading data for nodejs/node/pull/35003
✔  Done loading data for nodejs/node/pull/35003
----------------------------------- PR info ------------------------------------
Title      tools,doc: allow page titles to contain inline code (#35003)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:fix-module-page-title -> nodejs:master
Labels     commit-queue-failed, doc, tools
Commits    3
 - tools,doc: allow page titles to contain inline code
 - v12 compatible
 - Add tests and support for nested elements
Committers 1
 - Antoine du HAMEL 
PR-URL: https://github.com/nodejs/node/pull/35003
Reviewed-By: Richard Lau 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35003
Reviewed-By: Richard Lau 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-09-02T16:48:11Z: https://ci.nodejs.org/job/node-test-pull-request/33014/
- Querying data for job/node-test-pull-request/33014/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Mon, 31 Aug 2020 23:18:36 GMT
   ✔  Approvals: 2
   ✔  - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/35003#pullrequestreview-480703930
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35003#pullrequestreview-480855115
   ✖  This PR needs to wait 9 more minutes to land
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

github-actions · 2020-09-02T23:21:36Z

Commit Queue failed

- Loading data for nodejs/node/pull/35003 ✔ Done loading data for nodejs/node/pull/35003 ----------------------------------- PR info ------------------------------------ Title tools,doc: allow page titles to contain inline code (#35003) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch aduh95:fix-module-page-title -> nodejs:master Labels commit-queue-failed, doc, tools Commits 3 - tools,doc: allow page titles to contain inline code - v12 compatible - Add tests and support for nested elements Committers 1 - Antoine du HAMEL PR-URL: https://github.com/nodejs/node/pull/35003 Reviewed-By: Richard Lau Reviewed-By: Rich Trott ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/35003 Reviewed-By: Richard Lau Reviewed-By: Rich Trott -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-09-02T23:09:17Z: https://ci.nodejs.org/job/node-test-pull-request/33014/ - Querying data for job/node-test-pull-request/33014/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Mon, 31 Aug 2020 23:18:36 GMT ✔ Approvals: 2 ✔ - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/35003#pullrequestreview-480703930 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35003#pullrequestreview-480855115 -------------------------------------------------------------------------------- ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 35003 ✔ Downloaded patch to /home/runner/work/node/node/.ncu/35003/patch -------------------------------------------------------------------------------- .git/rebase-apply/patch:87: trailing whitespace. there to test just that. warning: 1 line adds whitespace errors. Applying: tools,doc: allow page titles to contain inline code Applying: v12 compatible Applying: Add tests and support for nested elements ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
tools,doc: allow page titles to contain inline code

Previously the HTML title would be cut to the first text node only.

PR-URL: #35003
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

[detached HEAD 8678760c] tools,doc: allow page titles to contain inline code
Author: Antoine du HAMEL duhamelantoine1995@gmail.com
Date: Tue Sep 1 01:12:54 2020 +0200
2 files changed, 6 insertions(+), 6 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
v12 compatible

PR-URL: #35003
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

[detached HEAD 84fdf8ac] v12 compatible
Author: Antoine du HAMEL duhamelantoine1995@gmail.com
Date: Tue Sep 1 01:52:05 2020 +0200
2 files changed, 2 insertions(+), 2 deletions(-)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Add tests and support for nested elements

PR-URL: #35003
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

[detached HEAD 1f606767] Add tests and support for nested elements
Author: Antoine du HAMEL duhamelantoine1995@gmail.com
Date: Wed Sep 2 11:43:04 2020 +0200
3 files changed, 24 insertions(+), 20 deletions(-)
create mode 100644 test/fixtures/document_with_special_heading.md

Successfully rebased and updated refs/heads/master.
✔ 8678760c6286cea5d13761512d46bfd9a36f6fea
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 3:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length
✖ 84fdf8acd201b08727cb19fead617cebcc5379bf
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ 1f60676731214cfa03c68ac2e68dad9d82ea7ddf
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
ℹ Please fix the commit message and try again.

DerekNonGeneric

It doesn't seem to me like you actually ran the test. From what I gather, the CI doesn't run the doctool tests (not sure why), but please ensure that the test case being added does work, please.

test/doctool/test-doctool-html.js

test/fixtures/document_with_special_heading.md

Co-authored-by: Derek Lewis <DerekNonGeneric@inf.is>

aduh95 · 2020-09-03T08:54:14Z

It doesn't seem to me like you actually ran the test.

Why would you say such a thing using such a phrasing? I'm contributing to Node.js in good faith, I'm sorry if I made you feel otherwise, but I did actually ran the test.

the CI doesn't run the doctool tests (not sure why)

Yes, I'm pretty sure it does (it's part of make test-doc). If you want to convince yourself, you can open a PR that breaks this test, and check if you see a red cross.

DerekNonGeneric · 2020-09-03T09:01:31Z

Why would you say such a thing using such a phrasing?

Sorry, I wasn't sure how else to phrase that, but I don't see how this could've been run and pass. Am I mistaken? That's possible.

If you want to convince yourself, you can open a PR that breaks this test, and check if you see a red cross.

I mean, this is pretty convincing already, but I may need to analyze this further.

node/tools/test.py

Lines 1512 to 1526 in 79ea22a

    
           # these suites represent special cases that should not be run as part of the 
        
           # default JavaScript test-run, e.g., internet/ requires a network connection, 
        
           # addons/ requires compilation. 
        
           IGNORED_SUITES = [ 
        
             'addons', 
        
             'benchmark', 
        
             'doctool', 
        
             'embedding', 
        
             'internet', 
        
             'js-native-api', 
        
             'node-api', 
        
             'pummel', 
        
             'tick-processor', 
        
             'v8-updates' 
        
           ]

It's not really your fault if the test runner is messed up and you weren't able to ensure the test worked properly.

Sorry if I made it seem that way…

aduh95 · 2020-09-03T09:17:18Z

I think that's the list of ignored suites for make jstest. But I'm just guessing, the discussion would be more productive if someone more familiar with the test tool wanted to chime in.

From what I can see, make test should run the doctool test suite:

node/Makefile

Lines 600 to 609 in 79ea22a

    
           .PHONY: test-doc 
        
           test-doc: doc-only lint ## Builds, lints, and verifies the docs. 
        
           	@if [ "$(shell $(node_use_openssl))" != "true" ]; then \ 
        
           		echo "Skipping test-doc (no crypto)"; \ 
        
           	else \ 
        
           		$(PYTHON) tools/test.py $(PARALLEL_ARGS) doctool; \ 
        
           	fi 
        
           	$(NODE) tools/doc/checkLinks.js .

node/Makefile

Lines 316 to 326 in 79ea22a

    
           .PHONY: test 
        
           # This does not run tests of third-party libraries inside deps. 
        
           test: all ## Runs default tests, linters, and builds docs. 
        
           	$(MAKE) -s tooltest 
        
           	$(MAKE) -s test-doc 
        
           	$(MAKE) -s build-addons 
        
           	$(MAKE) -s build-js-native-api-tests 
        
           	$(MAKE) -s build-node-api-tests 
        
           	$(MAKE) -s cctest 
        
           	$(MAKE) -s jstest

DerekNonGeneric

Thank you for incorporating my feedback and clarifying why the test does work. 😜

nodejs-github-bot · 2020-09-03T12:45:33Z

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

Trott · 2020-09-03T13:36:51Z

Landed in d2c5e89. Thanks so much!

Previously the HTML title would be cut to the first text node only. PR-URL: #35003 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>

Previously the HTML title would be cut to the first text node only. PR-URL: nodejs#35003 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>

tools,doc: allow page titles to contain inline code

cfb7329

Previously the HTML title would be cut to the first text node only.

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Aug 31, 2020

richardlau reviewed Aug 31, 2020

View reviewed changes

tools/doc/package.json Outdated Show resolved Hide resolved

v12 compatible

2518ba1

DerekNonGeneric reviewed Sep 1, 2020

View reviewed changes

tools/doc/html.js Show resolved Hide resolved

Add tests and support for nested elements

7734e54

richardlau approved these changes Sep 2, 2020

View reviewed changes

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

Trott approved these changes Sep 2, 2020

View reviewed changes

richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 2, 2020

richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2020

richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2020

DerekNonGeneric suggested changes Sep 3, 2020

View reviewed changes

Apply suggestions from code review

202b80a

Co-authored-by: Derek Lewis <DerekNonGeneric@inf.is>

DerekNonGeneric approved these changes Sep 3, 2020

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2020

This comment has been minimized.

Sign in to view

Trott removed the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Sep 3, 2020

Trott closed this Sep 3, 2020

richardlau mentioned this pull request Sep 7, 2020

v14.10.0 proposal #35023

Merged

4 tasks

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

aduh95 deleted the fix-module-page-title branch September 15, 2021 15:44

Uh oh!

Conversation

aduh95 commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

richardlau commented Sep 1, 2020

Uh oh!

aduh95 commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 2, 2020

Uh oh!

nodejs-github-bot commented Sep 2, 2020

Uh oh!

nodejs-github-bot commented Sep 2, 2020

Uh oh!

github-actions bot commented Sep 2, 2020

Uh oh!

github-actions bot commented Sep 2, 2020

Uh oh!

github-actions bot commented Sep 2, 2020

PR-URL: #35003 Reviewed-By: Richard Lau riclau@uk.ibm.com Reviewed-By: Rich Trott rtrott@gmail.com

PR-URL: #35003 Reviewed-By: Richard Lau riclau@uk.ibm.com Reviewed-By: Rich Trott rtrott@gmail.com

PR-URL: #35003 Reviewed-By: Richard Lau riclau@uk.ibm.com Reviewed-By: Rich Trott rtrott@gmail.com

Uh oh!

DerekNonGeneric left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 commented Sep 3, 2020

Uh oh!

DerekNonGeneric commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DerekNonGeneric left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Sep 3, 2020

Uh oh!

Trott commented Sep 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aduh95 commented Aug 31, 2020 •

edited

Loading

aduh95 commented Sep 2, 2020 •

edited

Loading

PR-URL: #35003
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

PR-URL: #35003
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

PR-URL: #35003
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Rich Trott rtrott@gmail.com

DerekNonGeneric commented Sep 3, 2020 •

edited

Loading

aduh95 commented Sep 3, 2020 •

edited

Loading