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.

Sign up for GitHub

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

Jump to bottom

tools: add unified plugin changing links for html docs #29946

Closed
wants to merge 4 commits into from
Closed

tools: add unified plugin changing links for html docs #29946

wants to merge 4 commits into from

Conversation

MarekLabuz
Copy link

This commit introduces additional stage in the process of generating
html docs from markdown files. Plugin transforms links to *.md files
in the respository to links to *.html files in the online documentation.

Fixes: #28689

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Oct 12, 2019
Trott
Trott reviewed Oct 12, 2019
View reviewed changes
tools/doc/markdown.js Outdated
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.
Copy link
Member

Choose a reason for hiding this comment

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

Unless the test itself is substantially copied from another file with this copyright boilerplate, the copyright boilerplate should be omitted from new files.

@Trott
Copy link
Member

Trott commented Oct 12, 2019

Welcome @MarekLabuz and thanks for the pull request!

@Trott
Copy link
Member

Trott commented Oct 16, 2019

@nodejs/documentation On the one hand, this will resolve the issue linked near the top. On the other hand, the whole requiring-bottom-references-that-are-unused-in-the-markdown file will be flagged by our lint rules as they currently stand and it's a bit non-intuitive.

At the moment, I'm feeling kind of -0 on it, maybe -0.5. I'd prefer something less obtrusive, more "just works". But I'm not sure we can realistically get there. I'm not concerned about broken links in the markdown-rendered docs. It would be nice if they weren't broken, but probably greater than 99% of our users read the HTML docs, not the rendered markdown in the GitHub interface or whatever.

I do appreciate the effort here and I'd like to see if there's a way to improve it a bit to something I'd be more +1 on.

@MarekLabuz
Copy link
Author

Alright, I get it, thank you for your feedback. There is also a possibility to move these references to a separate file e.g.

{
  "synopsis": {
    "Command Line Options": "cli.html#cli_command_line_options",
    "web server": "http.html"
  }
}

However, I wonder whether it will be worth to maintain it since, as you said, the vast majority of users don't use it.

@MarekLabuz MarekLabuz requested a review from Trott October 24, 2019 17:41
@Trott
Copy link
Member

Trott commented Oct 25, 2019

I think the problem here is pretty small and probably not worth any added complexity to our setup to solve, but others might feel differently. Anyone?

@Trott
Copy link
Member

Trott commented Nov 6, 2019

@nodejs/collaborators This could use some reviews.

@nodejs-github-bot
Copy link
Collaborator

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

@rvagg
Copy link
Member

rvagg commented Nov 6, 2019

needs a rebase I think https://ci.nodejs.org/job/node-test-commit/32422/console

I recall lots of "unified" errors in the without-ssl tests in earlier release lines, worth confirming that this passes that hurdle.

Marek Łabuz and others added 4 commits November 6, 2019 15:57
@Trott
tools: add unified plugin changing links for html docs
4c559aa 
This commit introduces additional stage in the process of generating
html docs from markdown files. Plugin transforms links to *.md files
in the respository to links to *.html files in the online documentation.

Fixes: nodejs#28689
@Trott
Update markdown.js
51b7c57
@Trott
Update document_with_links.md
401a989
@Trott
refactor replaceLinks plugin to use separate
90277e7 
linking json file
@Trott Trott force-pushed the replace-docs-links-unified-plugin branch from 3d1363a to 90277e7 Compare November 6, 2019 23:59
@Trott
Copy link
Member

Trott commented Nov 6, 2019

The rebase here seems to have gotten a bit confusing. I think I cleaned it up correctly, but please check.

@MarekLabuz
Copy link
Author

Yes, I think it is correct, thank you.

@nodejs-github-bot
Copy link
Collaborator

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

@rvagg
Copy link
Member

rvagg commented Nov 8, 2019

withoutssl passed, that's all I cared about 👍

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member

Landed in 62c61b7, thanks for the PR and sorry it took a while for somebody to get around to merging it!

@addaleax addaleax closed this Nov 30, 2019
addaleax pushed a commit that referenced this pull request Nov 30, 2019
@addaleax
tools: add unified plugin changing links for html docs
62c61b7 
This commit introduces additional stage in the process of generating
html docs from markdown files. Plugin transforms links to *.md files
in the respository to links to *.html files in the online documentation.

Fixes: #28689

PR-URL: #29946
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
@addaleax
tools: add unified plugin changing links for html docs
5a6f836 
This commit introduces additional stage in the process of generating
html docs from markdown files. Plugin transforms links to *.md files
in the respository to links to *.html files in the online documentation.

Fixes: #28689

PR-URL: #29946
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
Merged
targos pushed a commit that referenced this pull request Dec 5, 2019
@targos
tools: add unified plugin changing links for html docs
27f7f89 
This commit introduces additional stage in the process of generating
html docs from markdown files. Plugin transforms links to *.md files
in the respository to links to *.html files in the online documentation.

Fixes: #28689

PR-URL: #29946
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
Closed
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@MylesBorins
tools: add unified plugin changing links for html docs
da1e5ae 
This commit introduces additional stage in the process of generating
html docs from markdown files. Plugin transforms links to *.md files
in the respository to links to *.html files in the online documentation.

Fixes: #28689

PR-URL: #29946
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@Trott Trott Awaiting requested review from Trott

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Website Generator] Links must be .html for nodejs.org/api but they're actually .md on github
5 participants
@MarekLabuz @Trott @nodejs-github-bot @rvagg @addaleax