Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate PropertySet #102

Closed
samreid opened this issue Sep 23, 2016 · 67 comments
Closed

Deprecate PropertySet #102

samreid opened this issue Sep 23, 2016 · 67 comments

Comments

@samreid
Copy link
Member

samreid commented Sep 23, 2016

As discussed in #71 and #101, PropertySet creates more problems than it solves and should be deprecated. The recommended pattern is described here #71 (comment)

with reset and disposal as described here:
#71 (comment)

If anyone wants to argue that PropertySet should be kept/fixed/etc, that should happen here. Otherwise, this issue is about how and when we will move away from PropertySet. Some possibilities:
(a) Replace all PropertySet occurrences now up front, then delete PropertySet immediately.
(b) Leave existing PropertySet occurrences for now, but no new code will use PropertySet
(c) When PhET-iO instrumenting a simulation, refactor away from PropertySet

We also have the possibility to refactor PropertySet as @pixelzoom described in https://github.com/phetsims/phet-io/issues/640 as an intermediate solution.

@jessegreenberg @pixelzoom @aadish @andrewadare @jbphet @jonathanolson please comment below and we'll decide at an upcoming developer meeting.

@samreid
Copy link
Member Author

samreid commented Sep 28, 2016

@jonathanolson proposed keeping ES5 get/set in #71 (comment) and I responded in #71 (comment)

@samreid
Copy link
Member Author

samreid commented Sep 29, 2016

We have to decide in #71 before doing anything here.

andrewadare added a commit to phetsims/charges-and-fields that referenced this issue Oct 19, 2016
…pertySet -> Property for ElectricPotentialSensor
andrewadare added a commit to phetsims/charges-and-fields that referenced this issue Oct 19, 2016
andrewadare added a commit to phetsims/charges-and-fields that referenced this issue Oct 20, 2016
…pertySet -> Property for MeasuringTape; use set() and get() for properties
andrewadare added a commit to phetsims/charges-and-fields that referenced this issue Oct 20, 2016
@pixelzoom
Copy link
Contributor

Over in #71, we decided to move away from PropertySet. So it's constructor should be annotated as @deprectated.

@pixelzoom
Copy link
Contributor

PropertySet constructor and its public public functions as now annotated as @deprecated.

Labeling for developer meeting to decide how to proceed with refactoring away from PropertySet.

@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 3, 2016

The repositories below use PropertySet. Shall we make an issue for each repository?

common code:

  • joist
  • scenery-phet
  • sun
  • vegas

simulations:

  • area-builder (JB/DB)
  • arithmetic (JB/DB)
  • balancing-act (JB/DB)
  • balloons-and-static-electricity
  • bending-light (SR/DB)
  • blackbody-spectrum
  • build-a-molecule (JO)
  • calculus-grapher (JO)
  • capacitor-lab-basics
  • charges-and-fields (MV)
  • circuit-construction-kit-basics (SR/DB)
  • color-vision (CM)
  • curve-fitting
  • energy-skate-park-basics (SR)
  • faradays-law (JB/DB)
  • fluid-pressure-and-flow (MK)
  • forces-and-motion-basics (JG/MB)
  • fraction-comparison
  • fraction-matcher (SR)
  • friction (JB/DB)
  • gravity-and-orbits (JG/MB)
  • gravity-force-lab-basics
  • john-travoltage
  • least-squares-regression
  • masses-and-springs
  • molecule-shapes (DB)
  • molecules-and-light (JG/MB)
  • neuron (JB/DB)
  • ohms-law
  • optics-lab (MV)
  • pendulum-lab (JO)
  • projectile-motion
  • proportion-playground
  • protein-synthesis (SR)
  • resistance-in-a-wire
  • rutherford-scattering (JG/MB)
  • seasons (some intern sometime - CM)
  • sugar-and-salt-solutions (SR)
  • trig-tour (JG/MB)
  • wave-on-a-string

pixelzoom added a commit to phetsims/simula-rasa that referenced this issue Nov 3, 2016
@samreid
Copy link
Member Author

samreid commented Nov 3, 2016

There are 160 occurrences of PropertySet.call and 20 occurrences of new PropertySet.

pixelzoom added a commit to phetsims/blast that referenced this issue Nov 3, 2016
pixelzoom added a commit to phetsims/chains that referenced this issue Nov 3, 2016
pixelzoom added a commit to phetsims/vegas that referenced this issue Nov 3, 2016
@pixelzoom
Copy link
Contributor

I removed PropertySet from simula-rasa, blast, chains and vegas.

pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Nov 3, 2016
pixelzoom added a commit to phetsims/area-model-multiplication that referenced this issue Nov 3, 2016
@samreid
Copy link
Member Author

samreid commented Nov 3, 2016

@ariel-phet recommended to create 30+ issues, one per each repo. Even better: Let's add a note to the "how to instrument" doc to this effect.

@pixelzoom says we should fix the common code that uses propertyset. @pixelzoom will create issues for that.

@jbphet will tackle ButtonModel.

samreid added a commit to phetsims/fluid-pressure-and-flow that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/fluid-pressure-and-flow that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/sugar-and-salt-solutions that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/protein-synthesis that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/area-builder that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/estimation that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/molarity that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/ph-scale that referenced this issue Sep 13, 2017
samreid added a commit to phetsims/fluid-pressure-and-flow that referenced this issue Sep 14, 2017
samreid added a commit to phetsims/fluid-pressure-and-flow that referenced this issue Sep 14, 2017
samreid added a commit to phetsims/balancing-chemical-equations that referenced this issue Sep 14, 2017
samreid added a commit to phetsims/energy-skate-park-basics that referenced this issue Sep 14, 2017
samreid added a commit to phetsims/fluid-pressure-and-flow that referenced this issue Sep 14, 2017
samreid added a commit to phetsims/molecule-polarity that referenced this issue Sep 14, 2017
samreid added a commit to phetsims/molecule-shapes that referenced this issue Sep 14, 2017
samreid added a commit to phetsims/phet-info that referenced this issue Sep 14, 2017
@samreid
Copy link
Member Author

samreid commented Sep 14, 2017

I finished removing usages of PropertySet then I deleted PropertySet. I'm going to let Bayes use preventGetSet for a few days in Fluid Pressure and Flow before deleting that. Also leaving this issue open in case there is anything else to discuss about it. Is the next step to "eliminate" Events? #83 Eliminate is in quotes because we may keep it or a variant of it for scenery node.

UPDATE: #142 seems a better reference for emitter/events.

@samreid samreid assigned samreid and unassigned zepumph Sep 14, 2017
samreid added a commit to phetsims/fluid-pressure-and-flow that referenced this issue Sep 14, 2017
@pixelzoom
Copy link
Contributor

Agreed, #142 is the reference for converting Events to Emitter.

Looks like our work here is done?

Denz1994 added a commit to phetsims/molecule-shapes that referenced this issue Sep 14, 2017
@zepumph
Copy link
Member

zepumph commented Sep 14, 2017

Good work everyone, that was a biggy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests