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

tools: remove unused code in doc generation tool #32913

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 18, 2020

tools/doc/html.js includes code that looks for comments with
start-include and end-include for file inclusion. This seems to be
legacy code that is no longer used. The code only appears in the
table-of-contents generation function. The strings start-include and
end-include appear nowhere else in our tools and nowhere at all in our
docs.

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

tools/doc/html.js includes code that looks for comments with
`start-include` and `end-include` for file inclusion. This seems to be
legacy code that is no longer used. The code only appears in the
table-of-contents generation function. The strings `start-include` and
`end-include` appear nowhere else in our tools and nowhere at all in our
docs.
@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 18, 2020
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

I was talking to @mhdawson yesterday about the n-api compatibility matrix in the docs and the subject of whether it was possible to pull the compatibility table out so that it was included rather than embedded in the docs came up (as an attempt to ease keeping the table up to date across all release lines). Without having looked at the functionality being removed here in great detail (I wasn't aware of it before) perhaps it could be useful for that?

@Trott
Copy link
Member Author

Trott commented Apr 19, 2020

Without having looked at the functionality being removed here in great detail (I wasn't aware of it before) perhaps it could be useful for that?

@richardlau Sounds like you or @mhdawson should chime in over in #32916!

The code here only runs for table-of-contents production, so I don't think it will do what you need as-is. But it or some other technique could be repurposed elsewhere. (I don't think that should stop us from removing it for now.)

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 19, 2020
@DerekNonGeneric
Copy link
Contributor

I don't know what the policy is on removing dead code, but at the bare minimum I would probably add our discussion to the list of refs seeing as how removal of this would only be logical (from my limited perspective) if it were superseded by actually extending Markdown to support includes. I remember seeing that there was a discussion about the intention of the PR related to this work, but I didn't dig too deep.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell pushed a commit that referenced this pull request Apr 23, 2020
tools/doc/html.js includes code that looks for comments with
`start-include` and `end-include` for file inclusion. This seems to be
legacy code that is no longer used. The code only appears in the
table-of-contents generation function. The strings `start-include` and
`end-include` appear nowhere else in our tools and nowhere at all in our
docs.

PR-URL: #32913
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 23, 2020

Landed in 5315304

@jasnell jasnell closed this Apr 23, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
tools/doc/html.js includes code that looks for comments with
`start-include` and `end-include` for file inclusion. This seems to be
legacy code that is no longer used. The code only appears in the
table-of-contents generation function. The strings `start-include` and
`end-include` appear nowhere else in our tools and nowhere at all in our
docs.

PR-URL: #32913
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
tools/doc/html.js includes code that looks for comments with
`start-include` and `end-include` for file inclusion. This seems to be
legacy code that is no longer used. The code only appears in the
table-of-contents generation function. The strings `start-include` and
`end-include` appear nowhere else in our tools and nowhere at all in our
docs.

PR-URL: #32913
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 30, 2020
tools/doc/html.js includes code that looks for comments with
`start-include` and `end-include` for file inclusion. This seems to be
legacy code that is no longer used. The code only appears in the
table-of-contents generation function. The strings `start-include` and
`end-include` appear nowhere else in our tools and nowhere at all in our
docs.

PR-URL: #32913
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 13, 2020
tools/doc/html.js includes code that looks for comments with
`start-include` and `end-include` for file inclusion. This seems to be
legacy code that is no longer used. The code only appears in the
table-of-contents generation function. The strings `start-include` and
`end-include` appear nowhere else in our tools and nowhere at all in our
docs.

PR-URL: #32913
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott deleted the remove-unused-include branch April 14, 2022 11:28
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.

10 participants