You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by Mark Koudritsky <ka...@gmail.com> on 2014/01/17 20:36:30 UTC
Review Request 17064: Add a CordovaError exception class for better error
reporting in CLI
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/
-----------------------------------------------------------
Review request for cordova.
Bugs: CB-5782
https://issues.apache.org/jira/browse/CB-5782
Repository: cordova-cli
Description
-------
The on('uncaughtException') handler was removed recently in cordova-cli because
it was hiding the stack traces of some legit exceptions, but this results in
scary traces for simple errors like running outside a corodva project.
This change reintroduces the on('uncaughtException') handler and adds a special
CordovaError class for such simple errors. For exceptions of CordovaError class
only the message will be printed, for all other errors - the full stack trace.
Another pass over the code will be needed to find and convert the Errors to
CordovaErrors where appropriate. Will be done in a separate change.
Diffs
-----
src/cli.js d22ba56
src/util.js 278172a
Diff: https://reviews.apache.org/r/17064/diff/
Testing
-------
npm test
cordova plugin ls (inside and outside a a proj)
cordova bad_command
Thanks,
Mark Koudritsky
Re: Review Request 17064: Add a CordovaError exception class for
better error reporting in CLI
Posted by Andrew Grieve <ag...@chromium.org>.
Interesting. Makes sense if you've got the CordovaError at the bottom of a
file that uses, but I think it's moot if you put CordovaError into its own
module.
On Mon, Jan 20, 2014 at 2:20 PM, Mark Koudritsky <ka...@gmail.com> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17064/
>
> On January 20th, 2014, 1:53 a.m. UTC, *Andrew Grieve* wrote:
>
> src/util.js<https://reviews.apache.org/r/17064/diff/1/?file=427259#file427259line221> (Diff
> revision 1)
>
> 221
>
> this.constructor.prototype.__proto__ = Error.prototype
>
> No need to do this within the function. You should do it after defining the function:
> CordovaError.prototype.__proto__ = ...
>
> The argument the guy from the stack overflow link gives for assigning the prototype inside the constructor, is that this way you can put the definition wherever you want. If prototype assignment is done outside the func, you must be careful to put it before any other code in the same file wants to throw that error. The case seems relatively rare, but would probably lead to a pretty obscure bug. Not sure how strong this argument is.
>
>
> - Mark
>
> On January 17th, 2014, 7:36 p.m. UTC, Mark Koudritsky wrote:
> Review request for cordova.
> By Mark Koudritsky.
>
> *Updated Jan. 17, 2014, 7:36 p.m.*
> *Bugs: * CB-5782 <https://issues.apache.org/jira/browse/CB-5782>
> *Repository: * cordova-cli
> Description
>
> The on('uncaughtException') handler was removed recently in cordova-cli because
> it was hiding the stack traces of some legit exceptions, but this results in
> scary traces for simple errors like running outside a corodva project.
>
> This change reintroduces the on('uncaughtException') handler and adds a special
> CordovaError class for such simple errors. For exceptions of CordovaError class
> only the message will be printed, for all other errors - the full stack trace.
>
> Another pass over the code will be needed to find and convert the Errors to
> CordovaErrors where appropriate. Will be done in a separate change.
>
> Testing
>
> npm test
> cordova plugin ls (inside and outside a a proj)
> cordova bad_command
>
> Diffs
>
> - src/cli.js (d22ba56)
> - src/util.js (278172a)
>
> View Diff <https://reviews.apache.org/r/17064/diff/>
>
Re: Review Request 17064: Add a CordovaError exception class for better
error reporting in CLI
Posted by Mark Koudritsky <ka...@gmail.com>.
> On Jan. 20, 2014, 1:53 a.m., Andrew Grieve wrote:
> > src/util.js, line 221
> > <https://reviews.apache.org/r/17064/diff/1/?file=427259#file427259line221>
> >
> > No need to do this within the function. You should do it after defining the function:
> > CordovaError.prototype.__proto__ = ...
The argument the guy from the stack overflow link gives for assigning the prototype inside the constructor, is that this way you can put the definition wherever you want. If prototype assignment is done outside the func, you must be careful to put it before any other code in the same file wants to throw that error. The case seems relatively rare, but would probably lead to a pretty obscure bug. Not sure how strong this argument is.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/#review32272
-----------------------------------------------------------
On Jan. 17, 2014, 7:36 p.m., Mark Koudritsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17064/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 7:36 p.m.)
>
>
> Review request for cordova.
>
>
> Bugs: CB-5782
> https://issues.apache.org/jira/browse/CB-5782
>
>
> Repository: cordova-cli
>
>
> Description
> -------
>
> The on('uncaughtException') handler was removed recently in cordova-cli because
> it was hiding the stack traces of some legit exceptions, but this results in
> scary traces for simple errors like running outside a corodva project.
>
> This change reintroduces the on('uncaughtException') handler and adds a special
> CordovaError class for such simple errors. For exceptions of CordovaError class
> only the message will be printed, for all other errors - the full stack trace.
>
> Another pass over the code will be needed to find and convert the Errors to
> CordovaErrors where appropriate. Will be done in a separate change.
>
>
> Diffs
> -----
>
> src/cli.js d22ba56
> src/util.js 278172a
>
> Diff: https://reviews.apache.org/r/17064/diff/
>
>
> Testing
> -------
>
> npm test
> cordova plugin ls (inside and outside a a proj)
> cordova bad_command
>
>
> Thanks,
>
> Mark Koudritsky
>
>
Re: Review Request 17064: Add a CordovaError exception class for better
error reporting in CLI
Posted by Andrew Grieve <ag...@chromium.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/#review32272
-----------------------------------------------------------
Ship it!
Just two nits.
src/util.js
<https://reviews.apache.org/r/17064/#comment61007>
This should go in it's own file I think.
src/util.js
<https://reviews.apache.org/r/17064/#comment61006>
No need to do this within the function. You should do it after defining the function:
CordovaError.prototype.__proto__ = ...
- Andrew Grieve
On Jan. 17, 2014, 7:36 p.m., Mark Koudritsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17064/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 7:36 p.m.)
>
>
> Review request for cordova.
>
>
> Bugs: CB-5782
> https://issues.apache.org/jira/browse/CB-5782
>
>
> Repository: cordova-cli
>
>
> Description
> -------
>
> The on('uncaughtException') handler was removed recently in cordova-cli because
> it was hiding the stack traces of some legit exceptions, but this results in
> scary traces for simple errors like running outside a corodva project.
>
> This change reintroduces the on('uncaughtException') handler and adds a special
> CordovaError class for such simple errors. For exceptions of CordovaError class
> only the message will be printed, for all other errors - the full stack trace.
>
> Another pass over the code will be needed to find and convert the Errors to
> CordovaErrors where appropriate. Will be done in a separate change.
>
>
> Diffs
> -----
>
> src/cli.js d22ba56
> src/util.js 278172a
>
> Diff: https://reviews.apache.org/r/17064/diff/
>
>
> Testing
> -------
>
> npm test
> cordova plugin ls (inside and outside a a proj)
> cordova bad_command
>
>
> Thanks,
>
> Mark Koudritsky
>
>
Re: Review Request 17064: Add a CordovaError exception class for better
error reporting in CLI
Posted by Andrew Grieve <ag...@chromium.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/#review32417
-----------------------------------------------------------
Ship it!
Merged!
- Andrew Grieve
On Jan. 20, 2014, 10:07 p.m., Mark Koudritsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17064/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2014, 10:07 p.m.)
>
>
> Review request for cordova.
>
>
> Bugs: CB-5782
> https://issues.apache.org/jira/browse/CB-5782
>
>
> Repository: cordova-cli
>
>
> Description
> -------
>
> The on('uncaughtException') handler was removed recently in cordova-cli because
> it was hiding the stack traces of some legit exceptions, but this results in
> scary traces for simple errors like running outside a corodva project.
>
> This change reintroduces the on('uncaughtException') handler and adds a special
> CordovaError class for such simple errors. For exceptions of CordovaError class
> only the message will be printed, for all other errors - the full stack trace.
>
> Another pass over the code will be needed to find and convert the Errors to
> CordovaErrors where appropriate. Will be done in a separate change.
>
>
> Diffs
> -----
>
> src/CordovaError.js PRE-CREATION
> src/cli.js d22ba56
> src/util.js 278172a
>
> Diff: https://reviews.apache.org/r/17064/diff/
>
>
> Testing
> -------
>
> npm test
> cordova plugin ls (inside and outside a a proj)
> cordova bad_command
>
>
> Thanks,
>
> Mark Koudritsky
>
>
Re: Review Request 17064: Add a CordovaError exception class for better
error reporting in CLI
Posted by Mark Koudritsky <ka...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/
-----------------------------------------------------------
(Updated Jan. 20, 2014, 10:07 p.m.)
Review request for cordova.
Changes
-------
Style fix: removed the space after paren in "if".
Bugs: CB-5782
https://issues.apache.org/jira/browse/CB-5782
Repository: cordova-cli
Description
-------
The on('uncaughtException') handler was removed recently in cordova-cli because
it was hiding the stack traces of some legit exceptions, but this results in
scary traces for simple errors like running outside a corodva project.
This change reintroduces the on('uncaughtException') handler and adds a special
CordovaError class for such simple errors. For exceptions of CordovaError class
only the message will be printed, for all other errors - the full stack trace.
Another pass over the code will be needed to find and convert the Errors to
CordovaErrors where appropriate. Will be done in a separate change.
Diffs (updated)
-----
src/CordovaError.js PRE-CREATION
src/cli.js d22ba56
src/util.js 278172a
Diff: https://reviews.apache.org/r/17064/diff/
Testing
-------
npm test
cordova plugin ls (inside and outside a a proj)
cordova bad_command
Thanks,
Mark Koudritsky
Re: Review Request 17064: Add a CordovaError exception class for better
error reporting in CLI
Posted by Michal Mocny <mm...@chromium.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/#review32315
-----------------------------------------------------------
Ship it!
Ship It!
- Michal Mocny
On Jan. 20, 2014, 9:23 p.m., Mark Koudritsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17064/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2014, 9:23 p.m.)
>
>
> Review request for cordova.
>
>
> Bugs: CB-5782
> https://issues.apache.org/jira/browse/CB-5782
>
>
> Repository: cordova-cli
>
>
> Description
> -------
>
> The on('uncaughtException') handler was removed recently in cordova-cli because
> it was hiding the stack traces of some legit exceptions, but this results in
> scary traces for simple errors like running outside a corodva project.
>
> This change reintroduces the on('uncaughtException') handler and adds a special
> CordovaError class for such simple errors. For exceptions of CordovaError class
> only the message will be printed, for all other errors - the full stack trace.
>
> Another pass over the code will be needed to find and convert the Errors to
> CordovaErrors where appropriate. Will be done in a separate change.
>
>
> Diffs
> -----
>
> src/CordovaError.js PRE-CREATION
> src/cli.js d22ba56
> src/util.js 278172a
>
> Diff: https://reviews.apache.org/r/17064/diff/
>
>
> Testing
> -------
>
> npm test
> cordova plugin ls (inside and outside a a proj)
> cordova bad_command
>
>
> Thanks,
>
> Mark Koudritsky
>
>
Re: Review Request 17064: Add a CordovaError exception class for better
error reporting in CLI
Posted by Michal Mocny <mm...@chromium.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/#review32291
-----------------------------------------------------------
src/cli.js
<https://reviews.apache.org/r/17064/#comment61030>
Nit: s/fall/call/
src/cli.js
<https://reviews.apache.org/r/17064/#comment61031>
Trivial Nit: whitespace (leading space inside if condition, no space before previous line's brace)
- Michal Mocny
On Jan. 20, 2014, 9:23 p.m., Mark Koudritsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17064/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2014, 9:23 p.m.)
>
>
> Review request for cordova.
>
>
> Bugs: CB-5782
> https://issues.apache.org/jira/browse/CB-5782
>
>
> Repository: cordova-cli
>
>
> Description
> -------
>
> The on('uncaughtException') handler was removed recently in cordova-cli because
> it was hiding the stack traces of some legit exceptions, but this results in
> scary traces for simple errors like running outside a corodva project.
>
> This change reintroduces the on('uncaughtException') handler and adds a special
> CordovaError class for such simple errors. For exceptions of CordovaError class
> only the message will be printed, for all other errors - the full stack trace.
>
> Another pass over the code will be needed to find and convert the Errors to
> CordovaErrors where appropriate. Will be done in a separate change.
>
>
> Diffs
> -----
>
> src/CordovaError.js PRE-CREATION
> src/cli.js d22ba56
> src/util.js 278172a
>
> Diff: https://reviews.apache.org/r/17064/diff/
>
>
> Testing
> -------
>
> npm test
> cordova plugin ls (inside and outside a a proj)
> cordova bad_command
>
>
> Thanks,
>
> Mark Koudritsky
>
>
Re: Review Request 17064: Add a CordovaError exception class for better
error reporting in CLI
Posted by Mark Koudritsky <ka...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17064/
-----------------------------------------------------------
(Updated Jan. 20, 2014, 9:23 p.m.)
Review request for cordova.
Changes
-------
- Move CordovaError to CordovaError.js
- Move prototype assignment outside of the constructor.
Bugs: CB-5782
https://issues.apache.org/jira/browse/CB-5782
Repository: cordova-cli
Description
-------
The on('uncaughtException') handler was removed recently in cordova-cli because
it was hiding the stack traces of some legit exceptions, but this results in
scary traces for simple errors like running outside a corodva project.
This change reintroduces the on('uncaughtException') handler and adds a special
CordovaError class for such simple errors. For exceptions of CordovaError class
only the message will be printed, for all other errors - the full stack trace.
Another pass over the code will be needed to find and convert the Errors to
CordovaErrors where appropriate. Will be done in a separate change.
Diffs (updated)
-----
src/CordovaError.js PRE-CREATION
src/cli.js d22ba56
src/util.js 278172a
Diff: https://reviews.apache.org/r/17064/diff/
Testing
-------
npm test
cordova plugin ls (inside and outside a a proj)
cordova bad_command
Thanks,
Mark Koudritsky