Skip to content

Conversation

@shubheksha
Copy link
Contributor

Fixes#11273

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, errors, test

@nodejs-github-botnodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Feb 14, 2017
@jasnelljasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 14, 2017
Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

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

Almost there. Will need to look at one of the errors

lib/timers.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

hmm.. will have to think about this one because the error message is not going to format that well.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have separate error statements for non-finite and negative numbers

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. It would be a separate error code but that's workable

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. It would be a separate error code but that's workable

Copy link
Member

Choose a reason for hiding this comment

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

Note: both #11400 and #11390 touch this kind of error. Definitely +1 on having a error code dedicated for it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I misunderstood the discussion, it was about throwing different errors for <0 and isFinite, sorry. Anyway I still think there should be something like ERR_NEGATIVE_ARG

Copy link
Member

Choose a reason for hiding this comment

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

@shubheksha do you want to take a stab at addressing @joyeecheung 's comment? Thanks!

@ChALkeR
Copy link
Member

Blocked by #11580.

@ChALkeRChALkeR added blocked PRs that are blocked by other issues or PRs. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 27, 2017
@ChALkeR
Copy link
Member

#11580 landed, unblocking.

@jasnelljasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017
@fhinkel
Copy link
Member

@shubheksha Do you want to rebase this so we can pick this up again? Sorry for dragging it out for so long. Hope you're still interested in working on this!

@fhinkelfhinkel added the stalled Issues and PRs that are stalled. label Jun 7, 2017
@fhinkel
Copy link
Member

I'm closing this because it's been inactive for quite a while. Feel free to reopen or ping a collaborator to get it reopened if needed.

@fhinkelfhinkel closed this Jun 28, 2017
@shubheksha
Copy link
ContributorAuthor

Hey @fhinkel, I did rebase this.

@targostargos reopened this Jun 28, 2017
@refackrefack self-assigned this Jul 19, 2017
@refackrefack removed the blocked PRs that are blocked by other issues or PRs. label Jul 19, 2017
@BridgeAR
Copy link
Member

It seems like these errors were already migrated at some point. I am closing this therefore. @shubheksha I am sorry that your PR could not land and your work is much appreciated nevertheless!

@refackrefack removed their assignment Oct 20, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

errorsIssues and PRs related to JavaScript errors originated in Node.js core.semver-majorPRs that contain breaking changes and should be released in the next major version.stalledIssues and PRs that are stalled.timersIssues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@shubheksha@ChALkeR@fhinkel@BridgeAR@jasnell@Fishrock123@joyeecheung@refack@targos@nodejs-github-bot