phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

Problems with EnumerationProperty #374

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

A few of problems that I encountered when trying to use EnumerationProperty in geometric-optics:

(1) You can't set validValues, so you can't use EnumerationProperty with a subset of enumeration values:

type EnumerationPropertyOptions<T> = Omit<PropertyOptions<T>, 'validValues' | 'phetioType'>;

This is problematic for many sims where I've used enumerations, and blocks my use of EnumerationProperty in geometric-optics. Since PropertyOptions provides type-checking for validValues, I intended to make it an allowed option.

(2) These assertions are redundant, because these options are excluded by EnumerationPropertyOptions. They can be deleted.

assert && assert( !providedOptions || !providedOptions.hasOwnProperty( 'validValues' ), 'validValues is supplied by EnumerationProperty' );
assert && assert( !providedOptions || !providedOptions.hasOwnProperty( 'phetioType' ), 'phetioType is supplied by EnumerationProperty' );

(3) There's no need for {} as the first arg to merge, since validValues and phetioType can't possibly be in providedOptions. It would be better to put providedOptions as the last arg.

    const options = merge( {}, providedOptions, {
      validValues: value.enumeration!.values,
      phetioType: Property.PropertyIO( EnumerationIO<T>( {
        enumeration: value.enumeration!
      } ) )
    } );
pixelzoom commented 2 years ago

Problems addresses in the above commit.

@samreid would you mind reviewing, since you and I discussed this issue on Slack?

samreid commented 2 years ago

I reviewed the commit, and it seems good. Thanks also for removing the other assertion which is now unnecessary. Closing.