doc: grammar, clarity and links in timers doc by bengl · Pull Request #5792 · nodejs/node

bengl · 2016-03-18T20:20:00Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

doc

Description of change

In timers.markdown, dded appropriate in-document links. Clarified a bit of setImmediate, including a quick grammar fix (plural possessive apostrophe).

Added appropriate in-document links. Clarified a bit of `setImmediate`, including a quick grammar fix (plural possessive apostrophe).

jasnell · 2016-03-18T20:41:07Z

doc/api/timers.markdown

+To schedule the "immediate" execution of `callback` after I/O events'
+callbacks and before timers set by [`setTimeout`][] and [`setInterval`][] are
+triggered. Returns an `immediateObject` for possible use with
+[`clearImmediate`][]. Optionally you can also pass arguments to the callback.


Instead of using you here... perhaps: Additional optional arguments may be passed to the callback. or something similar.

I'd prefer to eliminate the use of the you pronoun in general (I see it's used several places in this doc)

Sure! That was in there previously, but now's as good a time as any to fix that.

jasnell · 2016-03-18T20:42:05Z

LGTM with a nit

Fishrock123 · 2016-03-18T20:43:55Z

doc/api/timers.markdown

 ## clearInterval(intervalObject)

-Stops an interval from triggering.
+Stops an `intervalObject`, as created by [`setInterval`][], from triggering.


This will also technically clear a regular timeout, I forget if the opposite is true, but the difference is really Immediates vs Timeouts, intervals are just a timeout with an extra property.

Ok, just confirmed, clearTimeout() does also work on intervals.

Is that worth documenting? I don't think I want to encourage folks to clear a timeout with clearInterval or vice-versa.

The actual objects are, for reference and naming, Timeout and Immediate.

@bengl I would refer to them by the names above, maybe call interval a "repeating Timeout", or "Interval (Timeout)"?

Hmmm, turns out Immediates actually have a constructor name and Timeouts do not, call timeouts and intervals whatever you want I guess, Immediate should probably be Immediate, though.

jasnell · 2016-03-18T21:07:46Z

@bengl ... LGTM! Thank you for updating that.

benjamingr · 2016-03-21T07:46:11Z

LGTM

Added appropriate in-document links. Clarified a bit of `setImmediate`, including a quick grammar fix (plural possessive apostrophe). PR-URL: #5792 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

benjamingr · 2016-03-21T07:50:08Z

Landed in 0fd3327 , thanks!

Refs: nodejs#5792 PR-URL: nodejs#5793 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

MylesBorins · 2016-03-21T19:52:13Z

@bengl is this related to the timer changes recently made by @Fishrock123 ?

bengl · 2016-03-21T19:59:51Z

@thealphanerd I think so, in that it seems @Fishrock123 was inspired by this PR to give Timeouts a proper constructor name so as to make them more documentable, etc.

Added appropriate in-document links. Clarified a bit of `setImmediate`, including a quick grammar fix (plural possessive apostrophe). PR-URL: #5792 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Refs: #5792 PR-URL: #5793 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Conflicts: test/message/timeout_throw.out

MylesBorins · 2016-03-30T19:26:09Z

@bengl to be clear, these changes are all applicable to lts right?

bengl · 2016-03-30T19:49:59Z

@thealphanerd I'd think so, yes.

Added appropriate in-document links. Clarified a bit of `setImmediate`, including a quick grammar fix (plural possessive apostrophe). PR-URL: #5792 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

The timers returned by `setTimeout` and friends are actually instances of `Timeout` and `Immediate`. Documenting them as such, so that the `ref` and `unref` methods can be identified as methods on `Timeout` objects. Sparked by discussion in nodejs#5792

@bengl

Overall improvements to timers.md documentation, Includes squashed commit from @bengl: doc: add timer classes The timers returned by `setTimeout` and friends are actually instances of `Timeout` and `Immediate`. Documenting them as such, so that the `ref` and `unref` methods can be identified as methods on `Timeout` objects. Sparked by discussion in nodejs#5792

@bengl

Overall improvements to timers.md documentation, Includes squashed commit from @bengl: doc: add timer classes The timers returned by `setTimeout` and friends are actually instances of `Timeout` and `Immediate`. Documenting them as such, so that the `ref` and `unref` methods can be identified as methods on `Timeout` objects. Sparked by discussion in #5792 PR-URL: #6937 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

@bengl

Overall improvements to timers.md documentation, Includes squashed commit from @bengl: doc: add timer classes The timers returned by `setTimeout` and friends are actually instances of `Timeout` and `Immediate`. Documenting them as such, so that the `ref` and `unref` methods can be identified as methods on `Timeout` objects. Sparked by discussion in #5792 PR-URL: #6937 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

doc: grammar, clarity and links in timers doc

3281780

Added appropriate in-document links. Clarified a bit of `setImmediate`, including a quick grammar fix (plural possessive apostrophe).

jasnell added doc Issues and PRs related to the documentations. lts-watch-v4.x labels Mar 18, 2016

jasnell reviewed Mar 18, 2016
View reviewed changes

Fishrock123 reviewed Mar 18, 2016
View reviewed changes

doc: remove "you" from timers doc

cc2d919

Fishrock123 mentioned this pull request Mar 18, 2016

timers: give Timeouts a constructor name #5793

Merged

2 tasks

mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 18, 2016

benjamingr closed this Mar 21, 2016

Fishrock123 mentioned this pull request Mar 22, 2016

v5.9.1 Release Proposal #5831

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

bengl mentioned this pull request Apr 14, 2016

doc: add timer classes #6206

Closed

2 tasks

Uh oh!

Conversation

bengl commented Mar 18, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 18, 2016

Uh oh!

benjamingr commented Mar 21, 2016

Uh oh!

benjamingr commented Mar 21, 2016

Uh oh!

MylesBorins commented Mar 21, 2016

Uh oh!

bengl commented Mar 21, 2016

Uh oh!

MylesBorins commented Mar 30, 2016

Uh oh!

bengl commented Mar 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants