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

doc: grammar, clarity and links in timers doc #5792

Closed
wants to merge 2 commits into from
Closed

doc: grammar, clarity and links in timers doc #5792

wants to merge 2 commits into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Mar 18, 2016

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).

@bengl
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 jasnell added doc Issues and PRs related to the documentations. lts-watch-v4.x labels Mar 18, 2016
jasnell
jasnell reviewed Mar 18, 2016
View reviewed changes
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jasnell
Copy link
Member

jasnell commented Mar 18, 2016

LGTM with a nit

Fishrock123
Fishrock123 reviewed Mar 18, 2016
View reviewed changes
doc/api/timers.markdown

## clearInterval(intervalObject)

Stops an interval from triggering.
Stops an `intervalObject`, as created by [`setInterval`][], from triggering.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

jasnell commented Mar 18, 2016

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

@Fishrock123 Fishrock123 mentioned this pull request Mar 18, 2016
Merged
2 tasks
@mscdex mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 18, 2016
@benjamingr
Copy link
Member

LGTM

benjamingr pushed a commit that referenced this pull request Mar 21, 2016
@bengl @benjamingr
doc: grammar, clarity and links in timers doc
0fd3327 
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
Copy link
Member

Landed in 0fd3327 , thanks!

@benjamingr benjamingr closed this Mar 21, 2016
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 21, 2016
@Fishrock123
timers: give Timeouts a constructor name
4fe02e2 
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
Copy link
Contributor

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

@bengl
Copy link
Member Author

bengl commented Mar 21, 2016

@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.

@Fishrock123 Fishrock123 mentioned this pull request Mar 22, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
@bengl @Fishrock123
doc: grammar, clarity and links in timers doc
f53cc37 
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>
Fishrock123 added a commit that referenced this pull request Mar 22, 2016
@Fishrock123
timers: give Timeouts a constructor name
20a68e9 
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
Copy link
Contributor

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

@bengl
Copy link
Member Author

bengl commented Mar 30, 2016

@thealphanerd I'd think so, yes.

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@bengl
doc: grammar, clarity and links in timers doc
16d3c1a 
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>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@bengl
doc: grammar, clarity and links in timers doc
040263e 
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>
@bengl bengl mentioned this pull request Apr 14, 2016
Closed
2 tasks
bengl added a commit to bengl/node that referenced this pull request May 6, 2016
@bengl
doc: add timer classes
f26db5b 
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 added a commit to bengl/node that referenced this pull request May 27, 2016
@bengl
doc: add timer classes
494bcc0 
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
jasnell added a commit to jasnell/node that referenced this pull request Jun 28, 2016
@jasnell
doc: general improvements to timers.md
4091f25 
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
jasnell added a commit that referenced this pull request Jun 28, 2016
@jasnell
doc: general improvements to timers.md
86e07b7 
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>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
@jasnell @Fishrock123
doc: general improvements to timers.md
bc37e6a 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bengl @jasnell @benjamingr @MylesBorins @Fishrock123 @mscdex