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

ShakerParticlesIO is calling private methods #223

Closed
pixelzoom opened this issue Dec 19, 2017 · 4 comments
Closed

ShakerParticlesIO is calling private methods #223

pixelzoom opened this issue Dec 19, 2017 · 4 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 19, 2017

Noted while working on #222.

ShakerParticlesIO is calling 2 private methods of ShakerParticles: removeAllParticles and addParticle. I never intended these method to be part of the public API. This needs to be fixed.

@samreid
Copy link
Member

samreid commented Jan 9, 2018

Would you be opposed to marking those methods as public (phet-io)? If so, can you suggest an alternate approach that would allow PhET-iO to set the state of the shaker particles?

Also, there is a chance that we may be able to eliminate the state of the shaker particles when we have the "Stable PhET-iO Design" for this simulation--if the design team decides that the shaker particles are ultimately transient and do not need to be recorded in the state.

EDIT: by that last token, I'd be fine to leave this issue open and on hold until the Stable PhET-iO Design meeting. Let me know what you prefer @pixelzoom

@samreid samreid assigned pixelzoom and unassigned samreid Jan 9, 2018
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 9, 2018

@samreid asked

Would you be opposed to marking those methods as public (phet-io)?

Simply changing their visibility annotation to @public is not appropriate. As noted in #223 (comment), they are @private because they were not designed to be called by clients. If we want them to be @public, I'll need to revisit the implementation, and investigate what changes are needed to make them @public.

I think we should leave this open until you've decided whether their state needs to be recorded. If their state does need to be recorded, then I'll need to revisit the implementation.

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Jan 9, 2018
@samreid
Copy link
Member

samreid commented Jan 10, 2018

Agreed, leaving open until after the stable PhET-iO design, unassigning for now.

@pixelzoom
Copy link
Contributor Author

ShakerParticlesIO was deleted as part of #244, so this issue can be closed.

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

2 participants