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

Docs: Fix broken links in Nodejs-api docs #13415

Closed
wants to merge 3 commits into from

Conversation

jooohhn
Copy link

@jooohhn jooohhn commented Jun 13, 2020

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

@jsf-clabot
Copy link

jsf-clabot commented Jun 13, 2020

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jun 13, 2020
kaicataldo
kaicataldo previously approved these changes Jun 13, 2020
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation and removed triage An ESLint team member will look at this issue soon labels Jun 13, 2020
Comment on lines 10 to 22
* [constructor()][eslint-constructor]
* [lintFiles()][eslint-lintFiles]
* [lintText()][eslint-lintText]
* [calculateConfigForFile()][eslint-calculateConfigForFile]
* [isPathIgnored()][eslint-isPathIgnored]
* [loadFormatter()][eslint-loadFormatter]
* [static version][eslint-version]
* [static outputFixes()][eslint-outputFixes]
* [static getErrorResults()][eslint-getErrorResults]
* [LintResult type](lintresult)
* [LintMessage type](lintmessage)
* [EditInfo type](editinfo)
* [Formatter type](formatter)
Copy link
Member

Choose a reason for hiding this comment

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

There are definitions of [eslint-constructor]-like links at the end of this file. Would you fix those instead? Those links are used in other places as well.

@mysticatea
Copy link
Member

Hmm.

The page in GitHub (/docs/developer-guide/nodejs-api.md) works fine. The root cause is, our website and GitHub markdown create different IDs for the markdown headers.

I wonder if we can fix our website's ID generator or not.

@jooohhn
Copy link
Author

jooohhn commented Jun 14, 2020

Updated table of contents to use new links at file end. It still has the problem you mentioned about Github docs and website docs having different header IDs. (can jump on website, but not on Github md)

@kaicataldo
Copy link
Member

We use AnchorJS to create these links, if someone wants to see if we can get the behavior to match how GitHub generates these links.

@kaicataldo kaicataldo dismissed their stale review July 3, 2020 19:52

We should fix the link generation of the site instead of breaking the Markdown docs.

@mdjermanovic
Copy link
Member

We could override AnchorJS's urlify, though I'm not sure if it's part of its public API.

Or, generate header ids upfront, which is supported by AnchorJS:

You can also use AnchorJS alongside a static navigation with pre-defined IDs

Or, maybe use a different library that supports custom functions that create ids, instead of AnchorJS.

As for the function, there's github-slugger. Though, I'm not sure if it matches what github is doing with that character we have on this page.

Comment on lines +19 to +22
* [LintResult type][lintresult]
* [LintMessage type][lintmessage]
* [EditInfo type][editinfo]
* [Formatter type][formatter]
Copy link
Member

Choose a reason for hiding this comment

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

👍

this was broken in markdown

@mdjermanovic
Copy link
Member

It seems that the main problem should be fixed in the eslint/website repo, but I think we should merge this PR first, because it converts uppercase links to lowercase, which is consistent with other documents and most likely something we'll rely on.

GitHub seems to have a script that handles any case, but we don't have that on the website. For example:

https://github.com/eslint/eslint/blob/master/docs/user-guide/configuring.md#Specifying-Processor (works)

https://eslint.org/docs/user-guide/configuring#Specifying-Processor (doesn't work)

Also, this PR fixes links that were broken for another reason (LintResult type, LintMessage type, EditInfo type, and Formatter type).

### new ESLint(options)
### new ESLint(options)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we want to remove

(though, removing these will probably lead to less possible problems later on the website)

Thoughts from the team?

Comment on lines +1385 to +1393
[eslint-constructor]: #new-eslint-options
[eslint-lintfiles]: #eslint-lintfiles-patterns
[eslint-linttext]: #eslint-linttext-code-options
[eslint-calculateconfigforfile]: #eslint-calculateconfigforfile-filepath
[eslint-ispathignored]: #eslint-ispathignored-filepath
[eslint-loadformatter]: #eslint-loadformatter-nameorpath
[eslint-version]: #eslint-version
[eslint-outputfixes]: #eslint-outputfixes-results
[eslint-geterrorresults]: #eslint-geterrorresults-results
Copy link
Member

Choose a reason for hiding this comment

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

These should be reverted to work well on GitHub (we should just convert uppercase to lowercase).

@mdjermanovic
Copy link
Member

The broken links have been fixed, so closing.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jan 2, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants