phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

required() is being used incorrectly #86

Closed zepumph closed 3 years ago

zepumph commented 4 years ago

required() was a function created over in https://github.com/phetsims/phet-info/issues/115. There are some usages though that are weird and buggy. Searching for : required( yields many cases where the function is being passed a constant value, like in SingleBulbPhotonBeam.js: phetioState: required( false ). false will always not be undefined. This should be cleaned up. Currently there are 23 usages of using required in an object literal. As described in https://github.com/phetsims/phet-info/issues/115#issuecomment-549524381, this is not ALWAYS buggy though, as this was an intended usage:

    config = merge( {

      // {number} How many decimal places should be shown
      decimalPlaces: required( config.decimalPlaces ),

      // {boolean} Whether we show options that let the user select the partial product style
      showProductsSelection: true,
     ...
    }, config );

Assigning to @ariel-phet to decide who would be good to go through usages and clean these up.

ariel-phet commented 4 years ago

@chrisklus is willing to tackle this issue. Might be good to do sooner than later just so it is out of the way.

chrisklus commented 4 years ago

In the above commits I either removed the use of required or passed in a config value, depending on what seemed most appropriate. I tested the affected sims before and after changes in their relevant brands. Assigning to @zepumph for review.

zepumph commented 4 years ago

Looks good to me, except https://github.com/phetsims/fractions-common/commit/5daf3de91f48786fe5b4d293942363513992a498, I'm not sure about that. Assigning to @jonathanolson to make sure and close if all is well.

jonathanolson commented 3 years ago

That commit looks good, seems like it fixes https://github.com/phetsims/fractions-common/commit/0ddc8a2f21dc7731fea01bc24d7c684b5ca5ea32. Closing.