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

getForString and getAllForString are being used incorrectly #23

Closed samreid closed 7 years ago

samreid commented 7 years ago

Discovered by me and @zepumph in https://github.com/phetsims/query-string-machine/issues/22, the existing usages of getForString and getAllForString are passing in a URL instead of the URL.search substring (which should be blank or start with a ?).

Problem sites: phet-io/stats.js phet-io/playback.js

The newly added assertion will fail these cases. But we should also consider making a QSM method that takes a URL instead of a location.search.

pixelzoom commented 7 years ago

While you're fixing things related to these functions...

Their function signatures strike me as odd, specifically the location of the string parameter. Compare to their less-specific counterparts (get and getAll):

get: function( key, schema )
getForString: function( string, key, schema )

getAll: function( schemaMap )
getAllForString: function( string, schemaMap )

When a function is made more specific by adding a parameter (string in this case), that parameter is typically added after the other parameters, not inserted before. Like this:

get: function( key, schema )
getForString: function( key, schema, string )

getAll: function( schemaMap )
getAllForString: function( schemaMap, string )
samreid commented 7 years ago

But we should also consider making a QSM method that takes a URL instead of a location.search.

The assertions+docs seem sufficient to prevent this need. I'll pursue @pixelzoom recommendation.

samreid commented 7 years ago

Fixed above, and tests passing. @pixelzoom would you like to review?

pixelzoom commented 7 years ago

Skimmed commits but didn't test drive, looks OK. Closing.