phetsims / fluid-pressure-and-flow

"Fluid Pressure and Flow" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
8 stars 5 forks source link

Deprecate PropertySet #295

Closed zepumph closed 7 years ago

zepumph commented 7 years ago

From https://github.com/phetsims/axon/issues/102, this issue is to track progress of PropertySet --> Property conversion in this repo.

I see 15 Usages of PropertySet.call( in this repo:

About 80 Properties in total.

My thoughts on process:

  1. I am not too familiar with PropertySet, but have spent some time looking into sims that use it.
  2. I take a small file and convert it (some file like Hose.js that just has two properties)
  3. I submit it to @samreid for review.
  4. We go from there. I think a pairing session to pick his brain on "best practices" would probably be best.

@samreid please state objections or comments. I think I will potentially try to do step 2 sometime later today.

samreid commented 7 years ago

Sounds OK, my tips for success are:

  1. do one property at a time (not one property set at a time)

For each one a. Make sure to use preventGetSet b. Search for all usages of the dynamic property c. Run a fuzz test to catch any you missed

Happy to meet when our times match up.

zepumph commented 7 years ago

Thanks! (And sorry for not assigning you when I wanted feedback)

zepumph commented 7 years ago

@samreid how does that look. I converted Hose.js from PropertySet. I ended up still using Events as the parent, which is deprecated, but I guess that's another job for this repo at some time (converting to emitters).

Also take your time. I won't be looking at this until at least Sept 14th.

samreid commented 7 years ago

I added

    Property.preventGetSet( this, 'angle' );
    Property.preventGetSet( this, 'height' );

and didn't see any issues during fuzz testing. Everything looks good. The only thing I would recommend to do differently is to leave in preventGetSet calls until everything is complete (in case different code gets exercised in a later use case), then we can remove them as a batch later on.

samreid commented 7 years ago

I've been tracking some of my commits for this over in https://github.com/phetsims/axon/issues/102

samreid commented 7 years ago

I finished this issue in the axon issue, closing.