On Wed, Nov 21, 2012 at 3:51 PM, Andrew Grieve <agri...@chromium.org> wrote: > Getting back to this - > > By coercing values, I mean if we're expecting a number, and they pass in > the string "3", then we have our helper method change it to a number > instead of throwing an exception. This would more closely match what > browser APIs do, but is maybe not worth the extra complexity. FWIW I hate API's that do this. I feel it is always correct to teach the user that they are making a mistake.
> > Okay, I'm going to go with throwing a TypeException, because I think it is > useful to make a distinction between a failed operation, and passing > invalid args. E.g. People's error callbacks will be easier to write if they > don't need to check the error code to see if they just messed up the call. > > > I'm going to get a start on this tomorrow, but will restrict the > refactoring to just a few plugins at first so that I'm not changing too > much before a release cut. > > > > > On Fri, Nov 16, 2012 at 4:13 AM, Brian LeRoux <b...@brian.io> wrote: > >> 1. I like the proposal Andrew. We need type checking for sure and making it >> optional for debugging even better. The light touch you have shown seems >> good enough for now. I'd rather we did not augment natives...and not so >> sure what you mean around coercing. >> >> 2. Success/error callbacks should be optional, says I. >> >> 3. When type errors happen, log & return, call errBack, or throw TypeError? >> >> I am very unsure about this one. Essentially this args checking is more for >> our bridge than it is for user space so browser api symmetry less important >> to me than doing right by the plugin author. It would seem this would >> indicate an log and error callback. But again, not entirely certain that >> will yield the most expected behavior. At least w/ throwing a TypeError we >> fail noisily. =/ >> >> >> >> >> On Fri, Nov 16, 2012 at 8:23 AM, Jesse MacFadyen <purplecabb...@gmail.com >> >wrote: >> >> > My answers/opinion >> > >> > 1. Up to the plugin developer >> > 2. Up to the plugin developer >> > 3. Up to the plugin developer >> > >> > However, that does not mean that we can't make the existing APIs >> > handle params a little more consistently. >> > >> > Also, we could expose typeChecking helper methods, although they are >> > pretty easy to come by. >> > Array.isArray() could be polyfil'd or could be cordova.isArray() ... >> > >> > Cheers, >> > Jesse >> > >> > Sent from my iPhone5 >> > >> > On 2012-11-15, at 12:16 PM, Andrew Grieve <agri...@google.com> wrote: >> > >> > > There's very little consistency when it comes to checking params in >> > plugin >> > > code. >> > > >> > > globalization.js< >> > >> https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/globalization.js;h=a57062ce8b1e0cc95dde54f8ca600f6ee4876bfd;hb=HEAD >> > >: >> > > checks every args. logs errors and returns without calling errback, >> does >> > > not allow missing errCB. >> > > >> > > resolveLocalFileSystemURI.js< >> > >> https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/resolveLocalFileSystemURI.js;h=c83b0aac3704c46782c3e19876afb180c32fc081;hb=HEAD >> > >: >> > > Allows missing successCB and errCB. Sanity checks URL has a colon in >> it. >> > > >> > > notification.js< >> > >> https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/notification.js;h=fbf9046a0e03bb6647219d7201b23f26f4d47084;hb=HEAD >> > >: >> > > doesn't check types at all >> > > >> > > contacts.js< >> > >> https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/contacts.js;h=2f5a51a20c8ad14b96c4ef935a28686a9bd87eca;hb=HEAD >> > >: >> > > Throws TypeError if missing successCB, allows missing errCB >> > > >> > > compass.js< >> > >> https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/compass.js;h=c97c1588bf69d2c94cde64736345a7d8b1dc32fe;hb=HEAD >> > >: >> > > Logs and returns if callbacks are of wrong type. success required, >> error >> > > optional. >> > > >> > > >> > > So, the things to agree upon are: >> > > >> > > 1. Should we check types or not? >> > > 2. Success / error callbacks : optional or not? >> > > 3. When type errors happen, log & return, call errBack, or throw >> > TypeError? >> > > >> > > >> > > >> > > For some extra inspiration, I played around with what it would look >> like >> > to >> > > at least factor out type checking code. Have a look at the >> > > module< >> > >> https://github.com/agrieve/incubator-cordova-js/blob/argscheck/lib/common/argscheck.js >> > >and >> > > it's >> > > tests< >> > >> https://github.com/agrieve/incubator-cordova-js/blob/argscheck/test/test.argscheck.js >> > >. >> > > Also, here's< >> > >> https://github.com/agrieve/incubator-cordova-js/blob/argscheck/lib/common/plugin/globalization.js >> > >what >> > > it looks like being used in globalization.js. >> > > >> > > >> > > Now, here's my thoughts for the three questions >> > > 1. Should we check types or not? >> > > - I think it is useful since incorrect args end up in exec() calls and >> > > are therefore harder to debug when they are wrong. >> > > - Perhaps instead of checking types, we add a helper for coercing >> types? >> > > This may only apply to strings / numbers though... >> > > - I also kind of like the idea of being able to turn off type checking >> > > for release mode. Having a common function to do the type checking >> would >> > > allow us to turn checking on/off for performance. >> > > >> > > 2. Success / error callbacks : optional or not? >> > > - I'm not positive we need to make these all consistent >> > > - If anything, I'd say we'd want them to be optional. >> > > >> > > 3. When type errors happen, log & return, call errBack, or throw >> > TypeError? >> > > - TypeError is my preference here. This matches what browser APIs do >> > when >> > > it doesn't like params. >> > >>
