Skip to content

docs: Fix broken fg(1) links#11504

Closed
karanjthakkar wants to merge 5 commits intonodejs:masterfrom
karanjthakkar:fix-fg-links
Closed

docs: Fix broken fg(1) links#11504
karanjthakkar wants to merge 5 commits intonodejs:masterfrom
karanjthakkar:fix-fg-links

Conversation

@karanjthakkar
Copy link
Contributor

@karanjthakkar karanjthakkar commented Feb 22, 2017

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

Fishrock123 commented Feb 22, 2017

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
Copy link
Contributor

@Fishrock123 Fishrock123 Feb 22, 2017

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

@karanjthakkar karanjthakkar Feb 22, 2017

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

@karanjthakkar karanjthakkar Feb 22, 2017

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

karanjthakkar commented Feb 22, 2017

@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

addaleax commented Feb 22, 2017

@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

karanjthakkar commented Feb 22, 2017

@addaleax Done :)

@addaleax
Copy link
Member

addaleax commented Feb 22, 2017

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

@thefourtheye
Copy link
Contributor

thefourtheye commented Feb 23, 2017

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

karanjthakkar commented Feb 23, 2017

@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

karanjthakkar commented Feb 23, 2017

@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

addaleax commented Feb 23, 2017

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

addaleax commented Feb 24, 2017

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

+1 more reviewer

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Reviewers whose approvals may not affect merge requirements

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