FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: remove redundant code from timers.js by Trott · Pull Request #3143 · nodejs/node · GitHub

/ node Public

lib: remove redundant code from timers.js - #3143

Closed
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:timer-tweak
ConversationCommits (3)ChecksFiles changed
Closed

lib: remove redundant code from timers.js#3143
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:timer-tweak

Conversation

Trott commented Oct 1, 2015

Copy link
Copy Markdown
Member

insert() is only called from one place where there is already a check
that msecs is greater than or equal to zero, so do not repeat the check
inside insert().

Trott added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 1, 2015

Trott commented Oct 1, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/407/

Fishrock123 commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

Seems to have originated along with the start of timers, ala pre- bc47353, perhaps insert was once used in more than one place. Not sure.

LGTM. Can you ensure there is something to test that negative timeouts do not fire?

thefourtheye commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

Can you leave this check and remove the one at the place where it is called?

Trott commented Oct 2, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 Regarding testing negative timeouts: test/parallel/test-timers.js has a bunch of tests using setTimeout() and setInterval() for various negative numbers. But the very first thing setTimeout() and setInterval() both do is change any value less than 1 to 1. So active() never actually gets the negative number. Does that cover the situation you are concerned about? Or not quite because of the guard code in setTimeout() and setInterval()?

Since active() is exported by timers.js, I can try to rig up a test calling it directly. But I dont' think active() should actually be exported. I can't find anything else that uses it, it's not documented, and the argument it requires is awfully specific to timers.js internal working, so I think it should be deprecated and made internal to timers.js. (But uh, that's a much bigger thing than I'd want to tackle with this small PR which I thought was going to be a two-line change...)

Trott commented Oct 2, 2015

Copy link
Copy Markdown
Member Author

@thefourtheye Yes, that is a slightly better approach. Thanks for suggesting it. I've made the change and pushed.

thefourtheye commented Oct 2, 2015

Copy link
Copy Markdown
Contributor

LGTM.

Fishrock123 reviewed Oct 2, 2015
View reviewed changes
Comment thread lib/timers.js Outdated

Fishrock123 Oct 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

isn't this line also redundant?

Trott Oct 2, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes. In fact, the whole separation of insert() from active() at this point seems redundant. Just pushed an additional commit.

Fishrock123 commented Oct 2, 2015

Copy link
Copy Markdown
Contributor

Yes, active is public but should not be -- see: #896

(I'll deal with this soon(tm).)

I'd add a test anyways since it currently is public, if the others don't already test it. I haven't looked.

Trott commented Oct 2, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 Per your suggestion, I've added a test for timers.active(). Thanks.

Trott commented Oct 3, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 Regarding #896, if (and only if) you want, I'd be happy to take the first step of that off your plate and make those APIs private within the file and replace the existing exposed functions with deprecation warning-wrapped versions and put it in a semver major PR.

Fishrock123 commented Oct 5, 2015

Copy link
Copy Markdown
Contributor

@Trott not worthwhile I think. They should be reworked at the same time. I'll look at it once I'm back from vacation.

Fishrock123 commented Oct 5, 2015

Copy link
Copy Markdown
Contributor

This LGTM if it works & CI is happy.

Trott commented Oct 5, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/423/

thefourtheye reviewed Oct 6, 2015
View reviewed changes
Comment thread test/parallel/test-timers-active.js Outdated

thefourtheye Oct 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not just hasOwnProperty? Also I feel that if we can check the values also it would be better.

thefourtheye Oct 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Shouldn't this check _idleStart also?

Fishrock123 Oct 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also note that we do now have notDeepStrictEqual :D

Trott Oct 6, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@thefourtheye @Fishrock123 OK, I've improved the tests quite a bit, I think. Looks good to you now?

Trott added 2 commits October 5, 2015 20:55
lib: remove redundant code, add tests in timers.js
58199b1 
insert() is only called from one place where there is already a check
that msecs is greater than or equal to zero, so do not repeat the check
inside insert().

timers.active() is not documented and should not be exposed, but since
it is exposed for now, let's test it.
fixup: improve test
4271293

Trott commented Oct 6, 2015

Copy link
Copy Markdown
Member Author

Made the tests a fair bit more rigorous with input from @thefourtheye and @Fishrock123. New CI: https://ci.nodejs.org/job/node-test-pull-request/426/

Fishrock123 reviewed Oct 6, 2015
View reviewed changes
Comment thread test/parallel/test-timers-active.js Outdated

Fishrock123 Oct 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

should, or should not?

Fishrock123 Oct 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

AH, this should be "should". :)

Trott Oct 6, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed the typo. Thanks for the catch.

thefourtheye reviewed Oct 6, 2015
View reviewed changes
Comment thread test/parallel/test-timers-active.js

thefourtheye Oct 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: these?

thefourtheye commented Oct 6, 2015

Copy link
Copy Markdown
Contributor

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Oct 6, 2015
lib: remove redundant code, add tests in timers.js
070aac4 
insert() is only called from one place where there is already a check
that msecs is greater than or equal to zero, so do not repeat the check
inside insert().

timers.active() is not documented and should not be exposed, but since
it is exposed for now, let's test it.

PR-URL: nodejs#3143
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott commented Oct 6, 2015

Copy link
Copy Markdown
Member Author

Landed in 070aac4

Trott closed this Oct 6, 2015
jasnell mentioned this pull request Oct 8, 2015
Closed
29 tasks
Trott added a commit that referenced this pull request Oct 8, 2015
lib: remove redundant code, add tests in timers.js
63644dd 
insert() is only called from one place where there is already a check
that msecs is greater than or equal to zero, so do not repeat the check
inside insert().

timers.active() is not documented and should not be exposed, but since
it is exposed for now, let's test it.

PR-URL: #3143
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell mentioned this pull request Oct 8, 2015
Closed
webzwo0i mentioned this pull request Oct 13, 2015
Closed
Trott deleted the timer-tweak branch January 13, 2022 22:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

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.

4 participants

Back | FazBrowse Home | New Git URL