Skip to content
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

Closed
pixelzoom opened this issue Oct 4, 2018 · 5 comments
Closed

replace RangeWithValue with Range where appropriate #78

pixelzoom opened this issue Oct 4, 2018 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

In #57, Range was split into Range and RangeWithValue. In #57 (comment), @mbarlow12 said:

To minimize disruption, RangeWithValue has replaced Range everywhere, even where only two parameters are passed in. Moving forward, the plan should be to gradually replace the existing RangeWithValue instances with Range when it can be verified that no default value is being used. ...

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.

pixelzoom added a commit to phetsims/acid-base-solutions that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@jonathanolson
Copy link
Contributor

It's something I've brought up in code reviews. I'm fine with a chip-away.

pixelzoom added a commit to phetsims/balancing-chemical-equations that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 4, 2018

How about doing this all at once using this approach? :

(1) Change anything new RangeWithValue that has 2 args to Range
(2) Add a temporary ES5 getter defaultValue to Range that errors when it's called, i.e.:

get defaultValue() { throw new Error( 'defaultValue is undefined' ); }

pixelzoom added a commit to phetsims/beers-law-lab that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/function-builder that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/molarity that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/ph-scale that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/reactants-products-and-leftovers that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor Author

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.

pixelzoom added a commit to phetsims/capacitor-lab-basics that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/sun that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/coulombs-law that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/curve-fitting that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/expression-exchange that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/fluid-pressure-and-flow that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/gravity-force-lab-basics that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/gravity-force-lab that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/inverse-square-law-common that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/isotopes-and-atomic-mass that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/sugar-and-salt-solutions that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/wave-on-a-string that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@pixelzoom
Copy link
Contributor Author

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 defaultValue a required parameter for RangeWithValue constructor:

  /**
   * @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.

pixelzoom added a commit to phetsims/ph-scale that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
pixelzoom added a commit to phetsims/capacitor-lab-basics that referenced this issue Oct 4, 2018
Signed-off-by: Chris Malley <cmalley@pixelzoom.com>
@jbphet
Copy link
Contributor

jbphet commented Oct 4, 2018

Reviewed in 10/4/2018 dev meeting, changes approved, awareness distributed, closing.

@jbphet jbphet closed this as completed Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants