timers: clarify lib/timer.js comment #11018

danbev · 2017-01-26T13:04:55Z

I stumbled a little when reading the following line:
"Timeouts only need to process any timers due to currently timeout"
and want to suggest changing currently -> the current.

The rest of the changes are only formatting to adhere to the 80
characters code style.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

timers

lance · 2017-01-26T15:12:43Z

LGTM

Fishrock123

I still hold by the original wording. It is more correct if perhaps not the most obvious.

Maybe the following would be better:

Timeouts only need to process any timers scheduled to timeout by the time the list is being processed,

lance

@Fishrock123 that is better, but I find "by the time the list is being processed" to be a little clunky. How about

For the reasons stated above, a timeout only need to process timeouts from the beginning of the timeout list. Any timers after the first one encountered that does not yet need to timeout will also always be due to timeout at a later time.

cjihrig · 2017-01-26T16:52:25Z

How about changing:

Timeouts only need to process any timers due to currently timeout

to:

Timeouts only need to process timers currently due to expire

Trott · 2017-01-26T18:05:11Z

Yet another option: Remove the word currently and change timeout to time out.

currently doesn't add information. There is no difference in meaning between "due to currently time out" and "due to time out".
At the same time as not adding any information, currently seems to be a source of confusion. So that's two reasons to remove it.
It gets rid of a split infinitive. Strictly speaking, split infinitives aren't wrong, but they should be used with care. Three reasons to remove currently.
As for timeout -> time out: This adds a little clarity because timeout is a noun and time out is a verb. "The timer is going to time out in 5 ms if you set a timeout of 5."

So: Timeouts only need to process any timers due to time out is still a little awkward but is a little bit in the right direction?

EDIT: Actually, @cjihrig's suggestion above is even better, I think.

danbev · 2017-01-29T07:14:24Z

@Fishrock123 @lance Would you guys also be okay with @cjihrig suggestion?

lance · 2017-01-30T13:49:19Z

Do :thumbs: not count as LGTM? 😉

Yes that works for me.

danbev · 2017-01-30T19:12:56Z

Do :thumbs: not count as LGTM? 😉

Sorry, I missed your thumps up :) Thanks for that!

Fishrock123 · 2017-01-30T19:28:47Z

For the reasons stated above, a timeout only need to process timeouts from the beginning of the timeout list. Any timers after the first one encountered that does not yet need to timeout will also always be due to timeout at a later time.

That could work.

I'm not seeing much difference in the current PR text than what is currently in master, aside form the user bit.

Keep in mind that timeout refers to when a a timer is in the process of calling a callback if one exists throughout the comments in other places. If we change one, maybe we should change the others?

It may be better to explain what that timeout operation means if we don't already, most other documentation of timers algorithms also refers to it as that if I recall correctly.

danbev · 2017-01-31T19:43:21Z

I'm not seeing much difference in the current PR text than what is currently in master

I realise my mistake here after re-reading it again a few times, and my original suggestion was just wrong. After reading it think the original is clear.

Though, it does seem like others also find the usage of currently could potentially confuse users as it confused me. Perhaps just removing currently as @Trott suggested would also do. That said I could go either way now and keep it like it is in master.

danbev · 2017-02-08T07:51:16Z

Keep in mind that timeout refers to when a a timer is in the process of calling a callback if one exists throughout the comments in other places. If we change one, maybe we should change the others?

@Fishrock123 If we go with @cjihrig suggestion I don't think we'd need to change anything other places.

It may be better to explain what that timeout operation means if we don't already, most other documentation of timers algorithms also refers to it as that if I recall correctly.

This line does sort of explain what a timeout is. Is this enough or should something else be added to the top of the comment section perhaps to make it clearer you think?

Clarifying and using cjihrig suggestion as mentioned here: nodejs#11018 (comment)

danbev · 2017-03-08T12:22:08Z

@Fishrock123 Sorry but I forgot to tag you on this before. Would you mind taking a look and see if you find the above acceptable?

danbev · 2017-03-27T06:41:28Z

@Fishrock123 Do you have any additional input on this issue?

fhinkel · 2017-05-26T19:00:02Z

ping @Fishrock123

BridgeAR · 2017-08-26T07:35:38Z

Ping @Fishrock123

Dimissing your review due to no response over a long time period. Please take another look.

BridgeAR · 2017-09-12T19:08:16Z

I will land this in a few days if there is no one objecting anymore.

BridgeAR · 2017-09-20T00:48:52Z

Landed in 3c65a83

PR-URL: #11018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Lance Ball <lball@redhat.com>

PR-URL: nodejs/node#11018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Lance Ball <lball@redhat.com>

PR-URL: #11018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Lance Ball <lball@redhat.com>

nodejs-github-bot added dont-land-on-v7.x timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Jan 26, 2017

jasnell approved these changes Jan 26, 2017

View reviewed changes

Fishrock123 previously requested changes Jan 26, 2017

View reviewed changes

lance reviewed Jan 26, 2017

View reviewed changes

mscdex removed the dont-land-on-v7.x label Jan 26, 2017

nodejs-github-bot added the lts-watch-v6.x label Jan 26, 2017

danbev closed this Feb 17, 2017

danbev reopened this Feb 17, 2017

danbev force-pushed the timers-comment-clarification branch from 4218f94 to c03c950 Compare February 17, 2017 07:09

nodejs-github-bot removed the lts-watch-v6.x label Feb 17, 2017

timers: clarify lib/timer.js comment

b36c429

Clarifying and using cjihrig suggestion as mentioned here: nodejs#11018 (comment)

danbev force-pushed the timers-comment-clarification branch from c03c950 to b36c429 Compare February 21, 2017 06:51

jasnell added the stalled Issues and PRs that are stalled. label Mar 27, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

BridgeAR approved these changes Aug 26, 2017

View reviewed changes

BridgeAR removed the stalled Issues and PRs that are stalled. label Sep 12, 2017

BridgeAR closed this Sep 20, 2017

BridgeAR pushed a commit that referenced this pull request Sep 20, 2017

timers: clarify lib/timer.js comment

3c65a83

PR-URL: #11018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Lance Ball <lball@redhat.com>

jasnell pushed a commit that referenced this pull request Sep 20, 2017

timers: clarify lib/timer.js comment

6a94c1a

PR-URL: #11018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Lance Ball <lball@redhat.com>

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

danbev deleted the timers-comment-clarification branch September 25, 2017 05:58

MylesBorins added the land-on-v6.x label Oct 17, 2017

MylesBorins pushed a commit that referenced this pull request Oct 17, 2017

timers: clarify lib/timer.js comment

e85cb9f

PR-URL: #11018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Lance Ball <lball@redhat.com>

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins pushed a commit that referenced this pull request Oct 25, 2017

timers: clarify lib/timer.js comment

f49feab

PR-URL: #11018 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Lance Ball <lball@redhat.com>

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

baby636 mentioned this pull request Jun 22, 2022

[Snyk] Security upgrade eslint from 5.16.0 to 6.6.0 baby636/node#59

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: clarify lib/timer.js comment #11018

timers: clarify lib/timer.js comment #11018

danbev commented Jan 26, 2017

lance commented Jan 26, 2017

Fishrock123 left a comment •

edited

Loading

lance left a comment •

edited

Loading

cjihrig commented Jan 26, 2017

Trott commented Jan 26, 2017 •

edited

Loading

danbev commented Jan 29, 2017

lance commented Jan 30, 2017

danbev commented Jan 30, 2017

Fishrock123 commented Jan 30, 2017 •

edited

Loading

danbev commented Jan 31, 2017

danbev commented Feb 8, 2017 •

edited

Loading

danbev commented Mar 8, 2017

danbev commented Mar 27, 2017

fhinkel commented May 26, 2017 •

edited

Loading

BridgeAR commented Aug 26, 2017

BridgeAR commented Sep 12, 2017 •

edited

Loading

BridgeAR commented Sep 20, 2017

timers: clarify lib/timer.js comment #11018

timers: clarify lib/timer.js comment #11018

Conversation

danbev commented Jan 26, 2017

Checklist

Affected core subsystem(s)

lance commented Jan 26, 2017

Fishrock123 left a comment • edited Loading

Choose a reason for hiding this comment

lance left a comment • edited Loading

Choose a reason for hiding this comment

cjihrig commented Jan 26, 2017

Trott commented Jan 26, 2017 • edited Loading

danbev commented Jan 29, 2017

lance commented Jan 30, 2017

danbev commented Jan 30, 2017

Fishrock123 commented Jan 30, 2017 • edited Loading

danbev commented Jan 31, 2017

danbev commented Feb 8, 2017 • edited Loading

danbev commented Mar 8, 2017

danbev commented Mar 27, 2017

fhinkel commented May 26, 2017 • edited Loading

BridgeAR commented Aug 26, 2017

BridgeAR commented Sep 12, 2017 • edited Loading

BridgeAR commented Sep 20, 2017

Fishrock123 left a comment •

edited

Loading

lance left a comment •

edited

Loading

Trott commented Jan 26, 2017 •

edited

Loading

Fishrock123 commented Jan 30, 2017 •

edited

Loading

danbev commented Feb 8, 2017 •

edited

Loading

fhinkel commented May 26, 2017 •

edited

Loading

BridgeAR commented Sep 12, 2017 •

edited

Loading