-
Notifications
You must be signed in to change notification settings - Fork 6
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
replace RangeWithValue with Range where appropriate #78
Comments
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
It's something I've brought up in code reviews. I'm fine with a chip-away. |
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
How about doing this all at once using this approach? : (1) Change anything
|
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
I completed conversion of all my sims and the common-code demos. Since that went so fast, I'll look at doing all other sims. |
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
All sims have been converted. I added this method to Range, to catch programming errors: /**
* In https://github.com/phetsims/dot/issues/57, defaultValue was moved to RangeWithValue.
* This ES5 getter catches programming errors where defaultValue is still used with Range.
*/
get defaultValue() {
throw new Error( 'defaultValue is undefined, did you mean to use RangeWithValue?' );
} And I made /**
* @param {number} min - the minimum value of the range
* @param {number} max - the maximum value of the range
* @param {number} defaultValue - default value inside the range
* @constructor
*/
function RangeWithValue( min, max, defaultValue ) {
Range.call( this, min, max );
assert && assert( defaultValue !== undefined, 'default value is required' );
assert && assert( defaultValue >= min && defaultValue <= max, 'defaultValue is out of range: ' + defaultValue );
// @private
this._defaultValue = defaultValue;
} Leaving this labeled for developer meeting so that developers are aware of these changes. |
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
Reviewed in 10/4/2018 dev meeting, changes approved, awareness distributed, closing. |
In #57, Range was split into Range and RangeWithValue. In #57 (comment), @mbarlow12 said:
The "gradually replace" part has apparently never happened. There are occurrences of RangeWithValue that should be using Range. That became obvious when reviewing change to Range and RangeWithValue in #77 (comment), wherein I proposed that the
defaultValue
parameter to RangeWithValue should be required.Should this be something that we "chip away" at? I'm planning to address it immediately in all of my sims, because I think it will take me < 30 minutes.
The text was updated successfully, but these errors were encountered: