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

unify valueType and phetioType options #194

Closed
pixelzoom opened this issue Nov 1, 2018 · 7 comments
Closed

unify valueType and phetioType options #194

pixelzoom opened this issue Nov 1, 2018 · 7 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

While we're doing #189 (Simplify Property type validation) ...

Let's consider unifying valueType and phetioType options. They are currently 2 very different ways of expressing identical semantics. Duplicating this information is undesirable for both instrumentation and maintainability.

Examples...

GRAPHING_QUADRATICS/GQModel

      // @public {Property.<Quadratic|null>} the saved quadratic, null if nothing is saved
      this.savedQuadraticProperty = new Property( null, {
        isValidValue: value => ( value instanceof Quadratic || value === null ),
        ...
        phetioType: PropertyIO( NullableIO( QuadraticIO ) ),
        ...
      } );

GRAPHING_QUADRATICS/PointTool

      // @public {Vector2}
      this.locationProperty = new Property( options.location, {
        valueType: Vector2,
        ...
        phetioType: PropertyIO( Vector2IO ),
        ...
      } );

AXON/BooleanProperty (and similarly in NumberProperty and StringProperty)

  function BooleanProperty( value, options ) {

    options = _.extend( {
      phetioType: PropertyIO( BooleanIO )
    }, options );

    ...
    options.valueType = 'boolean';  // BooleanProperty requires values to be primitive booleans

    Property.call( this, value, options );
  }
@zepumph
Copy link
Member

zepumph commented Nov 2, 2018

Wow what a great issue to create! We are looking into this for Emitter right now and it is going very well, see #190. To decide the main schema for determining what a "valid value" is, we will need to wait on the decision in #189

@pixelzoom
Copy link
Contributor Author

See also #195, proper handling of valueType: Array.

@samreid
Copy link
Member

samreid commented Dec 5, 2018

On hold until #189 is complete.

@samreid
Copy link
Member

samreid commented Jan 16, 2019

See also #204, which is the same issue for Emitters.

@pixelzoom
Copy link
Contributor Author

It seems like we addressed this in #189, where we concluded that these options will not be unified. @samreid @zepumph Can this issue be closed?

@samreid
Copy link
Member

samreid commented Aug 8, 2019

That sounds right to me, leaving up to @zepumph in case he has other thoughts or recommendations.

@samreid samreid removed their assignment Aug 8, 2019
@zepumph
Copy link
Member

zepumph commented Aug 8, 2019

where we concluded that these options will not be unified.

I think that you and I understand this in the same way @pixelzoom, though I want state the decision for this issue. Though they won't be unified via a mutual exclusion assertion, phetioType does cover valueType in most cases and, by convention, can/should be removed when the phetioType is provided. For example phetioType: NullableIO( BooleanIO ) is a superset of information of valueType: [ null, 'boolean' ], and so the valueType doesn't need to also be provided (and can be removed if already supplied). Closing

@zepumph zepumph closed this as completed Aug 8, 2019
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