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

ObjectIO method parameters are incorrect #183

Closed
zepumph opened this issue May 22, 2020 · 3 comments
Closed

ObjectIO method parameters are incorrect #183

zepumph opened this issue May 22, 2020 · 3 comments

Comments

@zepumph
Copy link
Member

zepumph commented May 22, 2020

Many of them are taylored to only reference serialization, for example toStateObject does not always get a PhetioObject, for Vector2 for example.

From slack:

Question about writing an IO Type like DataPointIO in projectile-motion.  ObjectIO.toStateObject documents that its param is of type {PhetioObject}.  But DataPoint does not extend PhetioObject.   So can toStateObject arg be any object? (edited) 






Michael Kauzmann:spiral_calendar_pad:  10:05
What about this from @chrisklus and I:
10:05
   * @param {PhetioObject|*} o - for most types of serialization this should be a PhetioObject, but for data type serialization, this can be anything, for more information see https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-guide.md#serialization

Chris Malley  10:07
That would be an improvement, and would probably make WebStorm happy.
10:08
For example, for BunnyIO:
/**
   * Serializes a Bunny to a state object.
   * @param {Bunny} bunny
   * @returns {Object}
   * @public
   * @override
   */
  static toStateObject( bunny ) {
    validate( bunny, this.validator );
    return bunny.toStateObject();
  }
... WebStorm flags bunny as "incompatible override, should have type 'PhetioObject'". (edited) 
10:09
Or maybe it should just be @param {*} o , because {PhetioObject|*} o is redundant. (edited) 
10:11
Same confusion for fromStateObject return value.  ObjectIO says that it returns {Object} But NumberIO is returning {number}, even though its jsdoc says {Object}. (edited) 

Michael Kauzmann:spiral_calendar_pad:  10:19
because {PhetioObject|*} o is redundant.
I am ok with the redundancy for clarity.

Michael Kauzmann:spiral_calendar_pad:  10:38
Maybe make it {*} but still keep the doc about PhetioObject?

Chris Malley  10:41
Yeah, that's what I was thinking.
@samreid
Copy link
Member

samreid commented Sep 12, 2020

This will be addressed by the proposal in #211, on hold until then.

@samreid
Copy link
Member

samreid commented Sep 26, 2020

These are centrally documented in IOType options:

      /**** STATE ****/

      // {function(coreObject:*):*)} Serialize the core object. Most often this looks like an object literal that holds
      // data about the PhetioObject instance.
      toStateObject: supertype && supertype.toStateObject,

      // {function(stateObject:*):*} For Data Type Deserialization. Decodes the object from a state (see toStateObject)
      // into an instance.
      fromStateObject: supertype && supertype.fromStateObject,

      // {function(stateObject:*):Array[*]} For Dynamic Element Deserialization: converts the state object to a `create`
      // function in PhetioGroup or other PhetioDynamicElementContainer creation function. Note that other non-serialized
      // args (not dealt with here) may be supplied as closure variables. This function only needs to be implemented on
      // IO Types that are phetioDynamicElement: true, such as PhetioGroup or PhetioCapsule elements.
      stateToArgsForConstructor: supertype && supertype.stateToArgsForConstructor,

      // {function(coreObject:*,stateObject:*)} For Reference Type Deserialization:  Applies the stateObject value to
      // the object. When setting PhET-iO state, this function will be called on an instrumented instance to set the
      // stateObject's value to it.
      // see https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-guide.md#three-types-of-deserialization
      applyState: supertype && supertype.applyState,

And methods that take an instance also run the validator. This issue seems ready to close.

@samreid samreid closed this as completed Sep 26, 2020
@samreid
Copy link
Member

samreid commented Feb 3, 2021

This issue is showing up in "open issue" searches. When I have seen that problem, before, it helps to reopen and reclose the issue.

@samreid samreid reopened this Feb 3, 2021
@samreid samreid closed this as completed Feb 3, 2021
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

3 participants