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

readline docs: adding events link #8475

Closed
wants to merge 2 commits into from
Closed

readline docs: adding events link #8475

wants to merge 2 commits into from

Conversation

italoacasas
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
  • doc
Description of change

I add two missing links to the readline documentation

@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 Sep 10, 2016
@mscdex
Copy link
Contributor

mscdex commented Sep 10, 2016

If you want, there are a couple of typos in that document that need to be fixed too (SIGSTP -> SIGTSTP and SIGTSPT -> SIGTSTP).

@mscdex
Copy link
Contributor

mscdex commented Sep 10, 2016

LGTM

@lpinca
Copy link
Member

lpinca commented Sep 10, 2016

Commit message does not follow commit guidelines.
I think something like "doc: link SIGTSTP / SIGCONT events in readline doc" should work.
@italoacasas can you please update it? Thanks!

@@ -90,7 +90,7 @@ The `'pause'` event is emitted when one of the following occur:

* The `input` stream is paused.
* The `input` stream is not paused and receives the `SIGCONT` event. (See
events `SIGTSTP` and `SIGCONT`)
events [SIGTSTP][] and [SIGCONT][])
Copy link
Member

Choose a reason for hiding this comment

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

Hm, these should probably keep their ```? I think the signal names are marked as code everywhere else here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax as a reader, I do not know when I'm reading the pause event where the other events are placed, but for some reason, the documentation tells me that they are related, I feel like we can improve the UX adding links instead ``. We are just adding a shortcut.

Copy link
Member

Choose a reason for hiding this comment

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

@italoacasas Maybe I’m misunderstanding you, but what I mean is that they should be links and code blocks, i.e. [SIGTSTP][] and [SIGCONT][] (and at the bottom of the file [SIGTSTP]: readline.html#readline_event_sigtstp, too). I’m totally agreeing that adding links makes sense here!

Copy link
Contributor Author

@italoacasas italoacasas Sep 10, 2016

Choose a reason for hiding this comment

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

😳 my bad. I was the one who did not understand you, it was clear what you meant in the first comment.

@jasnell
Copy link
Member

jasnell commented Sep 12, 2016

LGTM

1 similar comment
@lpinca
Copy link
Member

lpinca commented Sep 12, 2016

LGTM

lpinca pushed a commit that referenced this pull request Sep 13, 2016
PR-URL: #8475
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@lpinca
Copy link
Member

lpinca commented Sep 13, 2016

Landed in 53178f9, thanks!

@lpinca lpinca closed this Sep 13, 2016
Fishrock123 pushed a commit that referenced this pull request Sep 14, 2016
PR-URL: #8475
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants