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

replace SoluteIO ? #54

Closed
pixelzoom opened this issue Dec 10, 2018 · 6 comments
Closed

replace SoluteIO ? #54

pixelzoom opened this issue Dec 10, 2018 · 6 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 10, 2018

In SoluteIO.js:

//TODO replace this with StateObjectIO, see phetsims/phet-io#1100

phetsims/phet-io#1100 is closed and StateObjectIO no longer exists. Please advise on what needs to be done here.

pixelzoom added a commit that referenced this issue Dec 10, 2018
Signed-off-by: Chris Malley <[email protected]>
@pixelzoom pixelzoom changed the title replace SolutionIO ? replace SoluteIO ? Dec 11, 2018
@samreid
Copy link
Member

samreid commented Dec 11, 2018

I feel like SoluteIO is in good shape, it is not too complicated and what boilerplate code it does have, it is unclear how we would factor it out. Marking for discussion with @zepumph and @chrisklus on Wednesday to double check and discuss further.

@samreid samreid removed their assignment Dec 11, 2018
@zepumph
Copy link
Member

zepumph commented Dec 12, 2018

Discussed at phet-io team meeting today. We like that SoluteIO has a descriptive name and a place for type specific documentation. For example seeing
PropertyIO.<SoluteIO> would be much better in studio than PropertyIO.<StateObjectIO>

We do agree though that its reference-like qualities for serialization should be factored out. All the type checking can happen in a new type ReferenceIO.js, This type would know how to go from instance -> phetioID (toStateObject), and then from phetioID back to instance (fromStateObject). Here is what SoluteIO could look like afterwards.

// Copyright 2017-2018, University of Colorado Boulder

//TODO replace this with StateObjectIO, https://github.com/phetsims/molarity/issues/54
/**
 * IO type for Solute
 *
 * @author Chris Malley (PixelZoom, Inc.)
 */
define( function( require ) {
  'use strict';

  // modules
  var molarity = require( 'MOLARITY/molarity' );
  var ReferenceIO = require( 'TANDEM/types/ReferenceIO' );
  var phetioInherit = require( 'TANDEM/phetioInherit' );

  /**
   * @param {Solute} solute
   * @param {string} phetioID
   * @constructor
   */
  function SoluteIO( solute, phetioID ) {
    ReferenceIO.call( this, solute, phetioID );
  }

  phetioInherit( ReferenceIO, 'SoluteIO', SoluteIO, {}, {
    documentation: 'The solute for the sim',
  } );

  return molarity.register( 'SoluteIO', SoluteIO );
} );

Assigning to @samreid to investigate. Let us know if you need help.

@zepumph zepumph removed their assignment Dec 12, 2018
@phetsims phetsims deleted a comment from samreid Mar 26, 2019
@pixelzoom
Copy link
Contributor Author

@samreid @zepumph so is there anything to be done here? Or shall we close and revisit SoluteIO during PhET-iO redesign #51?

@samreid
Copy link
Member

samreid commented Apr 2, 2019

I added ReferenceIO and used it for SoluteIO. Seems to be working ok in studio=>launch. @zepumph can you take a look?

@zepumph
Copy link
Member

zepumph commented Apr 4, 2019

Things are looking pretty good while looking at the code. Full review is on hold until phetsims/tandem#85 is sort out.

@zepumph
Copy link
Member

zepumph commented Nov 13, 2020

This is all looking really good. Now SoluteIO looks like:


Solute.SoluteIO = new IOType( 'SoluteIO', {
  valueType: Solute,
  documentation: 'The solute for the sim',
  supertype: ReferenceIO( IOType.ObjectIO )
} );

@zepumph zepumph closed this as completed Nov 13, 2020
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