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

The application should fail to start if query parameter values are invalid (even if assertions are off). #4

Closed samreid closed 8 years ago

samreid commented 8 years ago

From #2

@pixelzoom said:

//TODO Since we don't have control over what the user enters for the parameter string, assertions herein should probably be changed to Errors

I opted for assertions because (a) they match the rest of PhET convention for argument checking (b) they can be stripped and (c) they are 1-line easy to read/write/maintain.

I think you are saying that we would want to prevent the application from launching if the allowed values are [1,2,3] and the user supplied [-1], for instance whether or not ?ea is specified. This seems like good reasoning.

samreid commented 8 years ago

In the preceding commit, I converted asserts to always error out during parsing. @pixelzoom would you like to review?

samreid commented 8 years ago

You can test this by running the test from line 32 of test-query-string-machine.html:

http://localhost/query-string-machine/test-query-string-machine.html?height=0
(should result in)
value not allowed: 0, allowedValues = 4,5,6,7,8
pixelzoom commented 8 years ago

If this is going to be useful to the end user, then the error message should identify which query parameter contains the error. You can do this by:

(1) Remove the console output from queryStringMachineAssert. (2) Catch the error in getForString and do the console output there, e.g.:

console.log( 'Error processing query parameter ' + key + ': ' + error.message );
samreid commented 8 years ago

In the preceding commit, QSM reports the key in the message like so:

console.log( 'Error for query parameter "' + key + '": ' + message )

I removed "processing" so the error is less likely to go off the page in chrome dev tools. I also passed the key to queryStringMachineAssert instead of throwing and catching, since it was more straightforward.

@pixelzoom would you like to review?

pixelzoom commented 8 years ago

Tried a few bogus query parameter values with function-builder and got expected error messages. Closing.