I appreciate that there is more to look at, I guess I would have just
preferred to see it in a feature branch.
I like it for the most part, the only issue I have is with this:
var argscheck = require('cordova/argscheck');
argscheck.checkArgs('fF', 'Device.getInfo', arguments);
I find this to be an increase in complexity ( granted it is small ),
at the expense of clarity.
I probably started a bigger conversation than I can sustain given that
I am on time off right now. I will be back at work on Monday and
happy to talk more then.
Cheers,
Jesse
On Thu, Nov 22, 2012 at 1:21 PM, Andrew Grieve <agri...@chromium.org> wrote:
> I don't mean to push ahead with this without your buy-in. What goes in can
> easily come out.
>
> I do want to give you concrete code to look at though, because I think our
> back-and-forth on this thread has made this change out to be more than it
> is.
>
>
> On Thu, Nov 22, 2012 at 4:02 PM, Jesse <purplecabb...@gmail.com> wrote:
>
>> Nevermind then, guess it's in ...
>>
>>
>>
>>
>>
>> On Thu, Nov 22, 2012 at 12:51 PM, Andrew Grieve <agri...@chromium.org>
>> wrote:
>> > I just checked in argscheck.js and refactored all applicable lower-case
>> > plugin/*.js files to use it.
>> >
>> > It trims 4k off of cordova.ios.js and git tells me:
>> > 245 insertions(+), 323 deletions(-)
>> >
>> > I also found that a couple of our tests were passing invalid arg types :P
>> >
>> >
>> > On Thu, Nov 22, 2012 at 3:48 PM, Andrew Grieve <agri...@chromium.org>
>> wrote:
>> >
>> >> There's a good amount of code that currently check types on the public
>> >> API. My goal here is to shrink that code because it seems repetitive.
>> >>
>> >> Checking the types passed to exec may be useful in some cases too, but
>> we
>> >> don't currently do that. Probably, if we wanted to add this in, we'd
>> want
>> >> something more sophisticated that actually checked JSON schemas instead
>> of
>> >> just checking for typeof 'object'.
>> >>
>> >>
>> >>
>> >>
>> >> On Thu, Nov 22, 2012 at 1:19 PM, Brian LeRoux <b...@brian.io> wrote:
>> >>
>> >>> Ok, hold up, I'm missing something---I thought this was for the *bridge
>> >>> protocol validation* not the actual API surface end developers invoke.
>> >>>
>> >>> Ideally the public API would define their own exceptional paths. (Har
>> >>> har.)
>> >>> Example: geolocation api has error callbacks whereas File API has
>> >>> FileError
>> >>> (or some such / on bad connection and just blasting this w/o checking).
>> >>>
>> >>>
>> >>> On Thu, Nov 22, 2012 at 2:01 PM, Patrick Mueller <pmue...@gmail.com>
>> >>> wrote:
>> >>>
>> >>> > On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
>> >>> >
>> >>> > > 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)
>> >>> > >
>> >>> >
>> >>> > huh? I thought this was all about informing users when they pass
>> >>> invalid
>> >>> > arguments?
>> >>> >
>> >>> > I'm usually in favor of "fail fast" - and so throwing an exception
>> when
>> >>> you
>> >>> > pass an invalid argument sounds right to me. The problem is that
>> even
>> >>> > though it's simple for us to fail fast by throwing an exception, we
>> also
>> >>> > need to make sure it's super obvious to the user that a failure has
>> >>> > occurred. That's the hard part. Too many places where errors are
>> >>> silently
>> >>> > consumed by the runtime.
>> >>> >
>> >>> > I think a console.log() would be appropriate - along with a thrown
>> >>> > exception - lots of folks have access to a "console" these days. Or
>> >>> maybe
>> >>> > we should come up with a new API - reportFailure() or something,
>> which
>> >>> we
>> >>> > could have - by default - just log to the console. For platforms
>> that
>> >>> > don't have an easily accessible console, they can override this to do
>> >>> > something visible for their platform.
>> >>> >
>> >>> > --
>> >>> > Patrick Mueller
>> >>> > http://muellerware.org
>> >>> >
>> >>>
>> >>
>> >>
>>
>>
>>
>> --
>> @purplecabbage
>> risingj.com
>>
--
@purplecabbage
risingj.com
