normalize getters and setters (fixes #3058) #3064
Conversation
|
@makepanic thanks |
boneskull
left a comment
boneskull
left a comment
There was a problem hiding this comment.
👍 just a couple tweaks and we can merge.
| */ | ||
| Runnable.prototype.slow = function (ms) { | ||
| if (typeof ms === 'undefined') { | ||
| if (!arguments.length || typeof ms === 'undefined') { |
There was a problem hiding this comment.
I think we can drop the ms check
There was a problem hiding this comment.
I've initially removed the ms check but tests were failing here.
I'm unsure if removing the ms check will introduce a breaking change.
There was a problem hiding this comment.
ugh, hum, let me think about this.
There was a problem hiding this comment.
that test is invalid and should be removed, but it's also a breaking change, so let's leave it for now.
There was a problem hiding this comment.
The reason why you test fails, is because when you invoke run.slow(undefined); argument.length is equal to 1. Instead of checking the number of arguments passed, you should just check if ms is undefined. You can do something like this:
if(!ms) {
....
}
There was a problem hiding this comment.
@fesebuv that's correct. but we just want to leave as-is for now. there's no change necessary here, since it'll be breaking.
lib/suite.js
Outdated
|
|
||
| /** | ||
| * Sets whether to bail after first error. | ||
| * Sets or gets whether to bail after first error. |
There was a problem hiding this comment.
maybe just Set or get like the others?
boneskull
added
the
semver-minor
| * @return {Context} self | ||
| */ | ||
| Context.prototype.enableTimeouts = function (enabled) { | ||
| if (!arguments.length) { |
There was a problem hiding this comment.
(fwiw, this is a breaking change for those who are consuming it directly. as it's undocumented, they shouldn't be.)
There was a problem hiding this comment.
So should i remove the arguments check from enableTimeouts?
There was a problem hiding this comment.
sorry to confuse; no, please leave it. I don't have any evidence this is used directly.
There was a problem hiding this comment.
(looks like you'll need to revert)
makepanic
force-pushed
the
issues/3058
branch
from
8a12604 to
adb3503
Compare
|
I've updated the PR to fix the jsdoc issue and remove the arguments check from |
makepanic
force-pushed
the
issues/3058
branch
from
adb3503 to
d959907
Compare
|
I've reverted removing the |
|
@makepanic thanks! |
4 participants
Description of the Change
This PR adds missing argument checks and adds a couple getters to some of the main classes.
Should fix #3058.
If there are additional getters that I've missed, I can add them.