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

docs: Fix broken fg(1) links #11504

Closed
wants to merge 5 commits into from
Closed

docs: Fix broken fg(1) links #11504

wants to merge 5 commits into from

Conversation

karanjthakkar
Copy link
Contributor

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

doc

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. labels Feb 22, 2017
@karanjthakkar karanjthakkar changed the title docs: Fix broken fg(1) links [WIP] docs: Fix broken fg(1) links Feb 22, 2017
@Fishrock123
Copy link
Contributor

1p? What does that even mean in the context of man docs?

Fishrock123
Fishrock123 suggested changes Feb 22, 2017
View reviewed changes
tools/doc/html.js Outdated
@@ -288,7 +288,7 @@ var BSD_ONLY_SYSCALLS = new Set(['lchmod']);
// Returns modified text, with such refs replace with HTML links, for example
// '<a href="http://man7.org/linux/man-pages/man2/open.2.html">open(2)</a>'
function linkManPages(text) {
return text.replace(/ ([a-z.]+)\((\d)\)/gm, function(match, name, number) {
return text.replace(/ ([a-z.]+)\((\d[a-z]?)\)/gm, function(match, name, number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we need both the original and new match for this to return http://man7.org/linux/man-pages/man1/fg.1p.html

currently it will return /man-pages/man1p/ by the looks of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fishrock123 Yup, I just noticed that. Fixing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fishrock123 Fixed this. Do you think there should be a test for this?

@Fishrock123 Fishrock123 mentioned this pull request Feb 22, 2017
Closed
@karanjthakkar
Copy link
Contributor Author

karanjthakkar commented Feb 22, 2017 •
edited
Loading

@Fishrock123 Haven't found any concrete documentation around it but this is what I found in the changelog. (Ref: http://man7.org/linux/man-pages/changelog.html#release_2.50)

In addition, man-pages provides the 1p pages, which document 
the portable subset of functionality of these commands.

@karanjthakkar
docs: Change broken fg(1) links to fg(1p)
c0429d1 
The fg(1) links in the readline docs have moved
from `http://man7.org/linux/man-pages/man1/fg.1.html`
to `http://man7.org/linux/man-pages/man1/fg.1p.html`.
It also modifies the regex for replacing man page links
in docs by allowing optional character after number.
eg: fg(1) and fg(1p) will both be now parsed and replaced.

Fixes: nodejs#11492
@karanjthakkar karanjthakkar changed the title [WIP] docs: Fix broken fg(1) links docs: Fix broken fg(1) links Feb 22, 2017
addaleax
addaleax approved these changes Feb 22, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

If you want, you can turn the function into an arrow function and the string concatenations into template strings, that would make the code a bit easier to read here.

This LGTM with or without that :)

@karanjthakkar
Copy link
Contributor Author

@addaleax The only reason I did not feel like changing it to fat arrow and template strings was because the code around it was still in the old syntax. Do you think I should still go ahead with it? I don't mind making that change.

@addaleax
Copy link
Member

@karanjthakkar We don’t really do bulk updates of code because that interferes with things like backporting and git blame, but if you are touching these lines anyway, you should feel free to do that, yes. :)

@karanjthakkar
Copy link
Contributor Author

@addaleax Done :)

@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/8082/

@thefourtheye
Copy link
Contributor

This change looks okay, but I found three more things in http://man7.org/linux/man-pages/dir_all_alphabetic.html, which will not be matched by this. 7stap, 3stap, and 3pcap. Not sure if we have to extend this to cover them as well.

@karanjthakkar
Copy link
Contributor Author

@thefourtheye thats interesting! But I couldn't seem to find any instances of man page references for those suffixes/commands being used inside the docs. If y'all think this PR should include that as well, then I can rework the regex to include that.

@karanjthakkar
Copy link
Contributor Author

@addaleax I've fixed the test for the json doctool. The CI build should pass now.

Fishrock123
Fishrock123 approved these changes Feb 23, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

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

@addaleax
Copy link
Member

If y'all think this PR should include that as well, then I can rework the regex to include that.

I think that would be a good idea, yes. (By the way, I read 1p as 1posix, and my local man expands it as that anyway.)

@addaleax
Copy link
Member

Landed in 6ae159f, resolved a minor conflict (neighbouring lines) while landing. Thanks for the PR! 🎉

@addaleax addaleax closed this Feb 24, 2017
addaleax pushed a commit that referenced this pull request Feb 24, 2017
@karanjthakkar @addaleax
doc: change broken fg(1) links to fg(1p)
6ae159f 
The fg(1) links in the readline docs have moved
from `http://man7.org/linux/man-pages/man1/fg.1.html`
to `http://man7.org/linux/man-pages/man1/fg.1p.html`.
It also modifies the regex for replacing man page links
in docs by allowing optional character after number.
eg: fg(1) and fg(1p) will both be now parsed and replaced.

Fixes: #11492
PR-URL: #11504
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
addaleax pushed a commit that referenced this pull request Feb 24, 2017
@karanjthakkar @addaleax
doc: change broken fg(1) links to fg(1p)
47034e1 
The fg(1) links in the readline docs have moved
from `http://man7.org/linux/man-pages/man1/fg.1.html`
to `http://man7.org/linux/man-pages/man1/fg.1p.html`.
It also modifies the regex for replacing man page links
in docs by allowing optional character after number.
eg: fg(1) and fg(1p) will both be now parsed and replaced.

Fixes: #11492
PR-URL: #11504
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
@karanjthakkar karanjthakkar deleted the fix-fg-links branch February 27, 2017 04:41
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

This will need a backport PR if it needs to land in v6 or v4

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

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@karanjthakkar @Fishrock123 @addaleax @thefourtheye @jasnell @nodejs-github-bot