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

ValidatorDef should support phetioType #258

Closed
zepumph opened this issue Aug 1, 2019 · 6 comments
Closed

ValidatorDef should support phetioType #258

zepumph opened this issue Aug 1, 2019 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented Aug 1, 2019

Over in #257 we thought this was the best way forward. Then we don't have to do a special case for the phetioType like we do in this sha here:

axon/js/Action.js

Lines 201 to 210 in 9e7e33a

for ( let i = 0; i < this.parameters.length; i++ ) {
const parameter = this.parameters[ i ];
if ( parameter.containsValidatorKeys ) {
validate( arguments[ i ], parameter );
}
// valueType overrides the phetioType validator so we don't use that one if there is a valueType
if ( parameter.phetioType && !this.parameters.valueType ) {
validate( arguments[ i ], parameter.phetioType.validator );
}

@zepumph
Copy link
Member Author

zepumph commented Aug 1, 2019

Above I added support for phetioType as a member of ValidatorDef. This was largely straightforward. I also added tests for this and found/fixed a bug in ArrayIO's validator.

The main piece I would like reviewed here is in Property.js. Up until this point, validator keys have been passed straight to Property, but have been used to validate the value the Property is set to, not the Property itself. But when adding support for phetioType, we were passing a phetioType like PropertyIO.<BooleanIO> into a validator setup to validate boolean, not Property.<boolean>. To get around this, I am no longer passing Property's options into validator, but instead I am picking out the validator keys into a validator var, and then overwriting the phetioType to be the parameterType of the phetioType (so BooleanIO instead of PropertyIO.<BooleanIO>). The main concern is that this adds one more object (to hold the validator) per Property instance. I do not know another way around it.

@samreid please review these changes, perhaps together?

@zepumph zepumph assigned samreid and unassigned zepumph Aug 1, 2019
zepumph added a commit to phetsims/circuit-construction-kit-common that referenced this issue Aug 2, 2019
zepumph added a commit to phetsims/masses-and-springs that referenced this issue Aug 2, 2019
zepumph added a commit to phetsims/circuit-construction-kit-common that referenced this issue Aug 2, 2019
zepumph added a commit to phetsims/masses-and-springs that referenced this issue Aug 2, 2019
@samreid
Copy link
Member

samreid commented Aug 27, 2019

b55a3bf seems reasonable. To alleviate our concerns about memory, why not only allocate _.pick( options, ValidatorDef.VALIDATOR_KEYS ) if assert is truthy?

@samreid samreid assigned zepumph and unassigned samreid Aug 27, 2019
@zepumph
Copy link
Member Author

zepumph commented Sep 3, 2019

How about just

     // Do not allocate the new object when assert is not enabled
      const validator = assert ? _.pick( options, ValidatorDef.VALIDATOR_KEYS ) : options;

That way we only make a new object if assert is false. What do you think?

@zepumph zepumph assigned samreid and unassigned zepumph Sep 3, 2019
samreid added a commit that referenced this issue Sep 4, 2019
@samreid
Copy link
Member

samreid commented Sep 4, 2019

That sounds good @zepumph. I also took that same idea a step further in the commit. Can you please review?

@zepumph
Copy link
Member Author

zepumph commented Sep 4, 2019

Oh that is so very nice. How do you like my minor tweak to remove duplicated documentation?

@samreid
Copy link
Member

samreid commented Sep 5, 2019

Looks great, thanks! I'm ready to close if you are.

@zepumph zepumph closed this as completed Sep 5, 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