ya upon further consideration making these TypeException's feels right since, ideally, this error would only be seen by a plugin author and not something a plugin consumer (ideally)
On Wed, Nov 21, 2012 at 8:53 PM, Michal Mocny <mmo...@chromium.org> wrote: > 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. > >> > > >> >
