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

Eliminate IOType.fromCoreType #273

Closed
samreid opened this issue Sep 29, 2022 · 6 comments
Closed

Eliminate IOType.fromCoreType #273

samreid opened this issue Sep 29, 2022 · 6 comments

Comments

@samreid
Copy link
Member

samreid commented Sep 29, 2022

From #263, I would like to eliminate IOType.fromCoreType. It was introduced as a convenience method, but it is making the type checking very complex and difficult, and making it difficult to make progress in classifying the different state serialization types.

We also now have well-established patterns and examples and type checking, so using the IOType constructor can be used safely. And it will be nice to have one way of doing things instead of two.

@samreid
Copy link
Member Author

samreid commented Sep 29, 2022

To make sure I didn't miss any use cases, I instrumented fromCoreType to output the different parts that are being passed through to the options. Here are the results:

AtomIO stateSchema

FourierComponentIO stateSchema
FourierComponentIO fromStateObject
FourierComponentIO toStateObject

EMEnergyPacketIO stateSchema
EMEnergyPacketIO fromStateObject
EMEnergyPacketIO toStateObject

EMWaveSourceIO stateSchema
EMWaveSourceIO coreTypeHasApplyState
EMWaveSourceIO toStateObject

EnergyInfoQueueItemIO stateSchema
EnergyInfoQueueItemIO fromStateObject

EnergyRateTrackerIO stateSchema

FrictionModelIO stateSchema

LayersModelIO stateSchema
LayersModelIO coreTypeHasApplyState
LayersModelIO toStateObject

PhotonCollectionIO stateSchema
PhotonCollectionIO coreTypeHasApplyState
PhotonCollectionIO toStateObject

PhotonIO stateSchema
PhotonIO fromStateObject
PhotonIO toStateObject

PlankIO stateSchema
PlankIO toStateObject

SpaceEnergySinkIO stateSchema

SunEnergySourceIO stateSchema

Vector2IO stateSchema
Vector2IO fromStateObject
Vector2IO toStateObject


WaveAtmosphereInteractionIO stateSchema
WaveAtmosphereInteractionIO fromStateObject
WaveAtmosphereInteractionIO toStateObject

WavesModelIO stateSchema
WavesModelIO coreTypeHasApplyState
WavesModelIO toStateObject

WaveAttenuatorIO stateSchema
WaveAttenuatorIO fromStateObject
WaveAttenuatorIO toStateObject

WaveCreationSpecIO stateSchema
WaveCreationSpecIO fromStateObject
WaveCreationSpecIO toStateObject

WaveIO stateSchema
WaveIO CoreType.stateToArgsForConstructor
WaveIO coreTypeHasApplyState
WaveIO toStateObject

BunnyIO coreTypeHasApplyState
BunnyIO CoreType.stateToArgsForConstructor
BunnyIO CoreType.STATE_SCHEMA

GenePairIO coreTypeHasApplyState
GenePairIO CoreType.STATE_SCHEMA

GenotypeIO coreTypeHasToStateObject
GenotypeIO coreTypeHasApplyState
GenotypeIO CoreType.STATE_SCHEMA

PhenotypeIO coreTypeHasToStateObject
PhenotypeIO coreTypeHasApplyState
PhenotypeIO CoreType.STATE_SCHEMA

BunnyCountsIO coreTypeHasToStateObject
BunnyCountsIO CoreType.fromStateObject
BunnyCountsIO CoreType.STATE_SCHEMA

ProportionsCountsIO coreTypeHasToStateObject
ProportionsCountsIO CoreType.fromStateObject
ProportionsCountsIO CoreType.STATE_SCHEMA

WolfIO coreTypeHasToStateObject
WolfIO coreTypeHasApplyState
WolfIO CoreType.stateToArgsForConstructor
WolfIO CoreType.STATE_SCHEMA

DataPointIO CoreType.fromStateObject
DataPointIO CoreType.STATE_SCHEMA

ProjectileObjectTypeIO coreTypeHasToStateObject
ProjectileObjectTypeIO coreTypeHasApplyState
ProjectileObjectTypeIO CoreType.STATE_SCHEMA

TrajectoryIO CoreType.stateToArgsForConstructor
TrajectoryIO CoreType.STATE_SCHEMA

RAPRatioTupleIO coreTypeHasToStateObject
RAPRatioTupleIO CoreType.fromStateObject
RAPRatioTupleIO CoreType.STATE_SCHEMA

samreid added a commit that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/projectile-motion that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/dot that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/natural-selection that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/greenhouse-effect that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/friction that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/balancing-act that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/ratio-and-proportion that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/fourier-making-waves that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/projectile-motion that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/greenhouse-effect that referenced this issue Sep 29, 2022
samreid added a commit to phetsims/natural-selection that referenced this issue Sep 29, 2022
@samreid
Copy link
Member Author

samreid commented Sep 29, 2022

Some notes from this issue:

TODO: Because serialization involves accessing
TODO: Why do some of these things just have stateSchema????

if it has either toStateObject or stateSchema, then it must have both

if it has toStateObject, it must have fromStateObject | applyState | {
stateToArgsForConstructor
applyState
}

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 30, 2022

Reopening.

Changes like this one in natural-selection violate PhET naming conventions.

- Bunny.BunnyIO = IOType.fromCoreType( 'BunnyIO', Bunny );
+ applyState: ( t, s ) => t.applyState( s ),

Abbreviated names are discouraged, especially names like t and s that convey no information.

I have a hunch that this is what it should be, but I'd have to dig around to be sure:

applyState: ( bunny, stateObject ) => bunny.applyState( stateObject ),

@samreid you did applyState: ( t, s ) 6 times in natural-selection, 1 time in projectile-motion. Please use more descriptive names.

@pixelzoom
Copy link
Contributor

pixelzoom commented Sep 30, 2022

@samreid I see similar naming violations for toStateObject and fromStateObject in the above commits. Please fix those too.

samreid added a commit that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/ratio-and-proportion that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/dot that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/molecule-shapes that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/greenhouse-effect that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/center-and-variability that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/natural-selection that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/projectile-motion that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/center-and-variability that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/balancing-act that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/projectile-motion that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/mobius that referenced this issue Oct 7, 2022
samreid added a commit to phetsims/natural-selection that referenced this issue Oct 7, 2022
@samreid
Copy link
Member Author

samreid commented Oct 7, 2022

Good idea, I improved the parameter names in the commits. Please let me know if I missed any.

@pixelzoom
Copy link
Contributor

Did you mean to assign this to me for review? I took a peek, looks like you got them all. Feel free to close.

@samreid samreid closed this as completed Oct 7, 2022
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