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, tools: make type parsing more strict #19881

Closed
wants to merge 2 commits into from
Closed

doc, tools: make type parsing more strict #19881

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 8, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

We do many small correction PRs for doc types format. This change makes type parsing more strict to avoid these post-corrections. Wrong or undefined types would throw on doc build stage.

Main code changes:

  • Make type check case-sensitive (no more uppercased primitives, lowercased global objects etc).
  • Make any type linkable (to this section).
  • Throw on any unrecognized type.

New parsing mode had detected some issues which have been fixed.

  • Fix miscased unlinkable types like {object} here.
  • Add missing type definitions for unlinkable types like {Cipher} here.
  • Fix undefined type synonyms in docs.
  • Replace non-JavaScript types with verbatim HTML fragments to be consistent in CSS style: only one place here.
  • Change type false-positive format here — notice the bizarre heading rendering and #hash URL part.

Also, some style nits in tools/doc/type-parser.js were fixed.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Apr 8, 2018
@vsemozhetbyt
Copy link
Contributor Author

* `args` {void\*} A pointer to pass to the callback at exit.
* `callback` <span class="type">&lt;void (\*)(void\*)&gt;</span>
A pointer to the function to call at exit.
* `args` <span class="type">&lt;void\*&gt;</span>
Copy link
Member

Choose a reason for hiding this comment

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

were these changed so they don't trigger the type parser?

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Apr 8, 2018

Choose a reason for hiding this comment

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

Yes. Currently, tools/doc/html.js type-process any non-code text in any section, so it cannot define if this is a js type or something else. But the author of this section wanted the C types to be consistent in CSS style with JS types, so I've tried to conform.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit.

typeLinks.push(`<span class="type">&lt;${typeTextFull}&gt;</span>`);
throw new Error(
`Unrecognized type: '${typeTextFull
}'. Please, edit the type or update the 'tools/doc/type-parser.js'.`
Copy link
Member

Choose a reason for hiding this comment

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

Please concat the string instead of using a multi line template string.

Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt Apr 8, 2018

Choose a reason for hiding this comment

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

Done in the fixup commit.

@vsemozhetbyt
Copy link
Contributor Author

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 8, 2018
@vsemozhetbyt
Copy link
Contributor Author

Rebased after the 0bd3da1.
CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/454/

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt added a commit that referenced this pull request Apr 11, 2018
PR-URL: #19881
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@vsemozhetbyt
Copy link
Contributor Author

Landed in b3bff41

@vsemozhetbyt vsemozhetbyt deleted the tools-doc-strict-types branch April 11, 2018 11:13
jasnell pushed a commit that referenced this pull request Apr 16, 2018
PR-URL: #19881
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
PR-URL: nodejs#19881
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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