Various problems with options types

phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.

GNU General Public License v3.0

0 stars 0 forks source link

Various problems with options types #219

Closed pixelzoom closed 5 months ago

pixelzoom commented 5 months ago

For code review #32 ...

In addition to problems noted in https://github.com/phetsims/projectile-data-lab/issues/216, there are a whole bunch of problems related to options types, including...

type names that don't match their associated class, e.g. MeasuresScreen and ProjectileDataLabScreenOptions
parent options type that does not match the superclass
swapped parameters to optionize (<Parent,Self,Provided> instead of <Provided,Self,Parent>)
missing uses of WithRequired<..., 'tandem'>
redundant usees of WithRequired<..., 'tandem'>
etc.

It will be easier for me to fix these than to enumerate them.

pixelzoom commented 5 months ago

I fixed many problems in https://github.com/phetsims/projectile-data-lab/commit/d01f7e204729ac3354d5646898fde4b2cc0983bb. There are probably more. Over to @samreid or @matthew-blackman to review.

Note that I did not do any narrowing of options types, nor will I open an issue about it, because I know that @samreid and I disagree on that topic. But I will note that this sim generally has options types that are unnecessarily broad, and sometimes dangerously broad -- allowing a client to blast something that is the responsibility of the associated class, and should not be part of the class' options API.

matthew-blackman commented 5 months ago

I went through each usage of optionize and checked that the correct options patterns were being applied. I caught 2 more cases that are fixed in the commits. I don't see any further work to be done here. Feel free to reopen if more errant options types pop up. Closing.