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

Several IO types should use ReferenceIO #215

Closed
samreid opened this issue Sep 29, 2020 · 5 comments
Closed

Several IO types should use ReferenceIO #215

samreid opened this issue Sep 29, 2020 · 5 comments

Comments

@samreid
Copy link
Member

samreid commented Sep 29, 2020

During #211 I noticed several IO types that were simulating ReferenceIO. I'll point their TODOs to this issue. There may be more as well. Also, keep in mind https://github.com/phetsims/phet-io/issues/1709#issuecomment-700805070 has a proposal that PhetioObject.toStateObject would be reference-ish by default.

@samreid samreid self-assigned this Sep 29, 2020
samreid added a commit to phetsims/forces-and-motion-basics that referenced this issue Sep 29, 2020
samreid added a commit to phetsims/masses-and-springs that referenced this issue Sep 29, 2020
samreid added a commit to phetsims/shred that referenced this issue Sep 29, 2020
samreid added a commit to phetsims/molecules-and-light that referenced this issue Nov 24, 2020
samreid added a commit to phetsims/forces-and-motion-basics that referenced this issue Nov 24, 2020
samreid added a commit to phetsims/masses-and-springs that referenced this issue Nov 24, 2020
samreid added a commit to phetsims/energy-skate-park that referenced this issue Nov 24, 2020
samreid added a commit to phetsims/shred that referenced this issue Nov 24, 2020
samreid added a commit to phetsims/sun that referenced this issue Nov 24, 2020
@samreid
Copy link
Member Author

samreid commented Nov 24, 2020

I converted these and tested briefly. I noted Energy Skate Park was failing before and after this change. @zepumph can you please spot check one or two of these? Close if it seems reasonable.

@zepumph
Copy link
Member

zepumph commented Dec 5, 2020

Isn't phetsims/energy-skate-park@c0d0a98 NullableIO( ReferenceIO( ObjectIO)?

@zepumph zepumph assigned samreid and unassigned zepumph Dec 5, 2020
@samreid
Copy link
Member Author

samreid commented Dec 7, 2020

The previous code does seem like it was supposed to be NullableIO, but looking at occurrences, it seems like it shouldn't be. ControlPoint has:

phetioType: ControlPoint.ControlPointIO,

which seems correct, and the snapTargetProperty is:

phetioType: Property.PropertyIO( NullableIO( ControlPoint.ControlPointIO ) ),

So even though I didn't port what was there, perhaps it turned out OK?

@samreid samreid assigned zepumph and unassigned samreid Dec 7, 2020
@zepumph
Copy link
Member

zepumph commented Dec 7, 2020

I see. OK sounds good if you feel good. Can you take one more glance for other cases like that, then feel free to close?

@zepumph zepumph assigned samreid and unassigned zepumph Dec 7, 2020
@samreid
Copy link
Member Author

samreid commented Dec 7, 2020

The other cases look like they are in the same boat: original version supported null, but didn't need to because it has a Property<Nullable<...>> elsewhere. Closing.

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