-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
Replaces DeveloperError(s) with Check(s) in Color.js #5424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just this suggestions:
Source/Core/Color.js
Outdated
} | ||
|
||
Check.typeOf.object('cartesian', cartesian); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout, remove any empty lines surrounding the check.
Source/Core/Color.js
Outdated
@@ -1,12 +1,14 @@ | |||
/*global define*/ | |||
define([ | |||
'./Check', | |||
'./defaultValue', | |||
'./defined', | |||
'./DeveloperError', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can now remove DeveloperError
from the list.
Source/Core/Color.js
Outdated
//>>includeEnd('debug'); | ||
|
||
Check.typeOf.number.lessThanOrEquals('minimumGreen', minimumGreen, maximumGreen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout, make sure the Check
call is between //>>includeStart('debug', pragmas.debug);
and //>>includeEnd('debug');
. The purpose of those is to mark code that should not be included in the release build.
Source/Core/Color.js
Outdated
throw new DeveloperError('alpha is required'); | ||
} | ||
|
||
Check.defined('color', color); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Check.typeOf.object
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look out for this with the rest of the checks. The one exception is for arrays which should continue to be checked with Check.defined
since we don't have a Check.typeOf.array
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'll make those changes
Source/Core/Color.js
Outdated
} | ||
|
||
Check.defined('color', color); | ||
Check.defined('alpha', alpha); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Check.typeOf.number
.
The fixes look good! |
Part of #4794