search

phetsims / query-string-machine

Query String Machine is a query string parser that supports type coercion, default values & validation. No dependencies.
MIT License
3 stars 3 forks source link

misplaced schema validation? #19

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

These checks appear in QueryStringMachine parseElement:

273 queryStringMachineAssert( !(schema.type && schema.parse), key, 'type and parse are mutually exclusive' );
274 queryStringMachineAssert( !(schema.validValues && schema.isValidValue), key, 'validValues and isValidValue are mutually exclusive' );
298 queryStringMachineAssert( VALID_TYPES.indexOf( schema.type ) >= 0, key, 'invalid type: ' + schema.type );

And this check appears in stringToArray:

208 queryStringMachineAssert( schema.elementSchema, key, 'array element schema must be defined' );

These are all related to schema validation. Shouldn't they be located in validateSchema?

pixelzoom commented 7 years ago

Looking more closely at the current state of QueryStringMachine... There's a lot of conditional logic in validateSchema and parseElement. And it's difficult to make a change (e.g. https://github.com/phetsims/chipper/issues/519) to the behavior of one type without affecting others. We could improve the readability and maintainability by breaking things up into type-specific validation and parsing functions. E.g., validateFlag and parseFlag, validateBoolean and parseBoolean, etc.

pixelzoom commented 7 years ago

The queryStringMachineAssert calls identified in https://github.com/phetsims/query-string-machine/issues/19#issue-191110685 should also probably be plain old assert. They are validating the schema, not a value provided by a user.

pixelzoom commented 7 years ago

Closed, move to https://github.com/phetsims/query-string-machine/issues/20.