Skip to content

Validate schema's default values #2511

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Mar 28, 2017
Merged

Validate schema's default values #2511

merged 17 commits into from
Mar 28, 2017

Conversation

@dye784
Copy link
Contributor

@dye784 dye784 commented Mar 22, 2017

Description:
What originally was intended to fix issue #2412 with PR #2500 grew into this PR. The intent of this is to enforce properly parsed default values based on a defined type instead of parsing the default into something acceptable.

Changes proposed:
Created validate.js which contains two functions: isValidDefaultCoordinate and isValidDefaultValue. isValidDefaultValue checks that the type and the typeof default value match. It calls isValidDefaultCoordinate to check vec2, vec3 and vec4. processPropertyDefinition throws a warning if isValidDefaultValue returns false.
@dye784 dye784 changed the title Validate schema Mar 22, 2017
@dye784
Copy link
Contributor Author

dye784 commented Mar 22, 2017

Noticed something is happening on default that creates something with

schema: {
  type: 'number',
  default: '1.0'
}

And that's giving a warning because of the functions I built.
dmarcos
dmarcos reviewed Mar 22, 2017
View reviewed changes
src/utils/validate.js Outdated
@@ -0,0 +1,61 @@
/**
* Checks if Valid default coordinates
* @param {unknown} possibleCoordinates
Copy link
Member

@dmarcos dmarcos Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we give to this file a more precise name? validate-schema.js, validate-type.js
Copy link
Contributor Author

@dye784 dye784 Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Thanks.
ngokevin
ngokevin requested changes Mar 22, 2017
View reviewed changes
src/core/schema.js Outdated
propDefinition.type = typeName;

// Fill in default value.
if (!('default' in propDefinition)) {
Copy link
Member

@ngokevin ngokevin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could swap the if around now so it's not negative:

if ('default' in propDefinition) {

} else {

}
src/utils/validate.js Outdated
// * @returns {boolean} A boolean determining if defaults are valid.
// */
function isValidDefaultValue (type, defaultVal) {
if (type === 'audio' && typeof defaultVal !== 'string') return false;
Copy link
Member

@ngokevin ngokevin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add braces around the if statements?
src/utils/validate-schema.js Outdated
@@ -0,0 +1,61 @@
/**
Copy link
Member

@ngokevin ngokevin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually just put all of this code within the schema module.
src/utils/validate-schema.js Outdated
* @returns {boolean} A boolean determining if coordinates are parsed correctly.
*/
function isValidDefaultCoordinate (possibleCoordinates, dimensions) {
if (typeof possibleCoordinates !== 'object' || possibleCoordinates === null) {
Copy link
Member

@ngokevin ngokevin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether null should be invalid.
Copy link
Contributor Author

@dye784 dye784 Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...that does make sense. Since currently if you do not define a default value, when you register a component it will register a default value at { x:0, y:0, z:0 } If you don't want that to happen assigning it to null makes sense. I'll adjust it.
src/utils/validate-schema.js Outdated
} else if (Object.keys(possibleCoordinates).length !== dimensions) {
return false;
} else {
if (dimensions === 2 && (possibleCoordinates.x === 0 || possibleCoordinates.x) && (possibleCoordinates.y === 0 || possibleCoordinates.y)) {
Copy link
Member

@ngokevin ngokevin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could condense this code more. We only need to check X/Y/Z/W each once.
Copy link
Contributor Author

@dye784 dye784 Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, I did it a dumb way. 😂
src/utils/validate-schema.js Outdated
* @returns {boolean} A boolean determining if defaults are valid.
*/
function isValidDefaultValue (type, defaultVal) {
if (type === 'audio' && typeof defaultVal !== 'string') return false;
Copy link
Member

@ngokevin ngokevin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Braces around the if code { }
Copy link
Contributor Author

@dye784 dye784 Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha.
src/utils/validate-schema.js Outdated
* @param {unknown} defaultVal - default in a prop
* @returns {boolean} A boolean determining if defaults are valid.
*/
function isValidDefaultValue (type, defaultVal) {
Copy link
Member

@ngokevin ngokevin Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we unit test these? (part of the schema.test.js)
Copy link
Contributor Author

@dye784 dye784 Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Was going to unit test it afterwards since it's quite a bit of tests to write. But I'll get on it.
@dye784
Copy link
Contributor Author

dye784 commented Mar 24, 2017

I've made all the changes you suggested @ngokevin. What do you think? Should I write some connection tests?
dye784
dye784 commented Mar 25, 2017
View reviewed changes
Copy link
Contributor Author

@dye784 dye784 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made all the changes suggested
@ngokevin ngokevin merged commit 57c314b into aframevr:master Mar 28, 2017
This was referenced Mar 29, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@dye784 @dmarcos @ngokevin