phetsims / phet-core

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

Add a module to assert options are mutually exclusive #60

Closed samreid closed 5 years ago

samreid commented 5 years ago

From https://github.com/phetsims/scenery-phet/issues/498#issuecomment-498109080

A note about mutually exclusive options. Slider.js already has this code:

// {Node} optional thumb, replaces the default.
// Client is responsible for highlighting, disabling and pointer areas.
// The thumb is positioned based on its center and hence can have its origin anywhere
thumbNode: null,

// Options for the default thumb, ignored if thumbNode is set
thumbSize: new Dimension2( 17, 34 ),
thumbFill: 'rgb(50,145,184)',
thumbFillHighlighted: 'rgb(71,207,255)',
thumbStroke: 'black',
thumbLineWidth: 1,
thumbCenterLineStroke: 'white',
thumbTouchAreaXDilation: 11,
thumbTouchAreaYDilation: 11,
thumbMouseAreaXDilation: 0,
thumbMouseAreaYDilation: 0,

I don't see any logic about requiring these options to be mutually exclusive. Similar options will be added for track, and I created a library for asserting the options are mutually exclusive. In the case of the track, the usage is like so:

if ( options ) {
  assert && assert( mutuallyExclusiveOptions( options, [ 'thumbNode' ], [
    'thumbSize', 'thumbFill', 'thumbFillHighlighted', 'thumbStroke', 'thumbLineWidth', 'thumbCenterLineStroke',
    'thumbTouchAreaXDilation', 'thumbTouchAreaYDilation', 'thumbMouseAreaXDilation', 'thumbMouseAreaYDilation'
  ] ), 'cannot provide both kinds of thumb options' );
}

This works great for Slider. However, NumberControl supplies these default options whether or not trackNode or thumbNode are passed in:

      trackSize: new Dimension2( 180, 3 ),
      thumbSize: new Dimension2( 17, 34 ),
      thumbTouchAreaXDilation: 6,

My choices are to (a) forego checks for mutual exclusivity until further discussion or (b) to only have NumberControl override defaults if thumbNode or trackNode are not supplied. I'm inclined to go with (b), but this will probably require discussion.

samreid commented 5 years ago

The mutual exclusivity feature is catching occurrences in

where thumbNode and thumbSize are both being specified. It seems nice to be able to detect and prevent this kind of problem.

samreid commented 5 years ago

From slack:

Sam Reid [12:56 PM] I’m looking for library support for mutually exclusive options. Like if options.a was passed in, then options.b and options.c must not be passed in. And conversely, if options.b OR options.c was passed in, then options.a cannot be. Is there some lodash support for that? Or what is a good starting point?

Jonathan Olson [12:57 PM] I’m not aware of any. I think once for 3 options I ended up adding a count of providing options and asserting on that

Sam Reid [1:16 PM] Sounds good, thanks!

Sam Reid [2:02 PM] Is this too obtuse?

assert && assert(
  Object.keys( _.pick( options, 'thumbNode' ) ).length *
  Object.keys( _.pick( options, 'thumbSize', 'thumbFill', 'thumbFillHighlighted', 'thumbStroke', 'thumbLineWidth',
    'thumbCenterLineStroke', 'thumbTouchAreaXDilation', 'thumbTouchAreaYDilation', 'thumbMouseAreaXDilation',
    'thumbMouseAreaYDilation' ) ).length === 0,
  'cannot provide both kinds of thumb options' 
);

Michael Kauzmann [2:05 PM] That looks pretty suave, what if it was hidden in a phetCore method like assertMutuallyExclusiveOptions( options, ...stringOrArrayOfString) where each subsequent arg is an key or group of keys that are mutually exclusive with all other args.

Sam Reid [2:09 PM] I literally have assert && assert( mutuallyExclusiveOptions( in my local history. We are thinking along the same lines!

Michael Kauzmann [2:09 PM] nice!

Jonathan Olson [2:10 PM] That sounds nice to me

Sam Reid [2:11 PM] For this case, it will read like:

assert && assert( mutuallyExclusiveOptions( options, [ 'thumbNode' ], [
      'thumbSize', 'thumbFill', 'thumbFillHighlighted', 'thumbStroke', 'thumbLineWidth', 'thumbCenterLineStroke',
      'thumbTouchAreaXDilation', 'thumbTouchAreaYDilation', 'thumbMouseAreaXDilation', 'thumbMouseAreaYDilation'
    ] ), 'cannot provide both kinds of thumb options' );

Michael Kauzmann [2:12 PM] so fun!

samreid commented 5 years ago

I found I could get a better assert message if switching to assertMutuallyExclusiveOptions, so I've moved in that direction.

samreid commented 5 years ago

I committed to master, this part is ready for review. Currently the only usage is in Slider.js which will be up for review soon as part of https://github.com/phetsims/scenery-phet/issues/498.

samreid commented 5 years ago

In https://github.com/phetsims/scenery-phet/issues/498#issuecomment-499654770, @pixelzoom said:

assertMutuallyExclusiveOptions looks generally useful, and appropriate to use here.

Anything else for this part? Do we need to survey other places to use this, or have a public service announcement about it?

pixelzoom commented 5 years ago

Do we need to survey other places to use this, or have a public service announcement about it?

I'm guessing that cost to survey other places to use this outweighs benefits. So I don't think it's necessary.

A PSA for new features is always useful.

Feel free to close.

samreid commented 5 years ago

Marking for dev meeting public service announcement.

UPDATE: To clarify, we now have a utility that throws assertion errors when mutually exclusive options are provided. Please search for assertMutuallyExclusiveOptions( for usage examples.

samreid commented 5 years ago

@ariel-phet requested to discuss this at another dev meeting when there are more developers.

ariel-phet commented 5 years ago

Marking as high priority so this gets PSA'ed

pixelzoom commented 5 years ago

8/8/19 dev meeting: @samreid will add doc for const assertMutuallyExclusiveOptions = function( options, ...sets ), then close.

samreid commented 5 years ago

Documented as prescribed, closing.