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

Rename this.tandem to prevent shadowing #212

Closed
samreid opened this issue Nov 4, 2017 · 11 comments
Closed

Rename this.tandem to prevent shadowing #212

samreid opened this issue Nov 4, 2017 · 11 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Nov 4, 2017

Please follow the conventions described in https://github.com/phetsims/phet-io/issues/1239

There is one occurrence in PrecipitateParticle and one in ShakerParticle

pixelzoom added a commit that referenced this issue Nov 4, 2017
@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 4, 2017

I renamed in PrecipitateParticle, ShakerParticle and TShakerParticle.

But in PrecipitateParticle:

35 this.precipitateParticleTandem = tandem; // Used in TPrecipitateParticle to serialize the tandem name

... and there is no use of the tandem in TPrecipitateParticle. So can this be deleted?

Assigning to @samreid, since he instrumented this sim.

@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

Yes, it seems safe to remove. I tested with phet-io fuzzMouse and didn't see any issues, so I committed the removal. Back to you @pixelzoom

@samreid samreid assigned pixelzoom and unassigned samreid Nov 4, 2017
@pixelzoom pixelzoom mentioned this issue Nov 4, 2017
@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 4, 2017

Next question... Why was a tandem field necessary for ShakerParticle, but not for PrecipitateParticle? Seems like their instrumentation would have identical requirements.

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Nov 4, 2017
@samreid
Copy link
Member Author

samreid commented Nov 4, 2017

I couldn't figure out why the tandem was in TShakerParticle, I removed it and the state wrapper still worked properly. I can't think of a reason that a researcher would need those ids in the state or anything else it might be used for. Still, to hedge my bets I'd like to double check with @zepumph before committing.

@samreid samreid assigned zepumph and unassigned samreid Nov 4, 2017
@zepumph
Copy link
Member

zepumph commented Nov 6, 2017

Sounds like an instrument everything policy to me. I'm not sure we need it, but if we do keep it, 4b77ee8 has a mistake in it. The toStateObject key tandem was not changed, but fromStateObject is now looking for a shakerParticleTandem not a tandem key.

@zepumph zepumph assigned samreid and pixelzoom and unassigned zepumph Nov 6, 2017
@pixelzoom
Copy link
Contributor

@zepumph does 3374339 fix the problem that you noted in #212 (comment)?

I certainly don't know whether shakerParticleTandem is needed. Perhaps we should just remove it for symmetry with PrecipitateParticle, so that they're either both correct or both wrong :)

@pixelzoom pixelzoom assigned zepumph and unassigned pixelzoom Nov 6, 2017
@zepumph
Copy link
Member

zepumph commented Nov 7, 2017

@zepumph does 3374339 fix the problem that you noted in #212 (comment)?

I don't think so, I thought it should look like
image

But I agree with you, perhaps we should delete it instead. There are phetioIDs on the instance anyways, so I'm voting delete.

@zepumph zepumph removed their assignment Nov 7, 2017
@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 7, 2017

OK, so you want to use shakerParticleTandem in the state object, as well as in ShakerParticle. If I interpret that correctly, then there's still a bug in #212 (comment) (preceding comment screenshot). stateObject.tandem should be stateObject.shakerParticleTandem, no?

pixelzoom added a commit that referenced this issue Nov 7, 2017
@pixelzoom
Copy link
Contributor

I deleted shakerParticleTandem from ShakerParticle and TShakerParticle in the previous commit.

@samreid please review.

@zepumph
Copy link
Member

zepumph commented Nov 7, 2017

If I interpret that correctly, then there's still a bug in #212 (comment) (preceding comment screenshot).

ARGH, you are very correct, sorry about that.

samreid added a commit that referenced this issue Nov 7, 2017
@samreid
Copy link
Member Author

samreid commented Nov 9, 2017

Looks like the tandems are not necessary in the state, I tested and confirmed the state wrapper still behaves nicely. Closing.

@samreid samreid closed this as completed Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants