search

AriaMinaei / pretty-error

See node.js errors with less clutter
MIT License
1.52k stars 49 forks source link

Replaces `uncaughtException` with `prepareStackTrace` #6

Closed alexgorbatchev closed 10 years ago

alexgorbatchev commented 10 years ago

uncaughtException doesn't work well with majority of test harnesses because they catch and display exceptions. This patch replaces uncaughtException console logging with a more "correct" way that better integrates into most stacks and leaves it up to the user to decide what to do with exceptions.

This is a direct port of functionality with a few additional tests.

Thoughts?

AriaMinaei commented 10 years ago

Wow, a fix with tests and everything! Thanks @alexgorbatchev!

I have a few urgent issues on my hand for this project. I'll get back to this as soon as I can :)

alexgorbatchev commented 10 years ago

I would be happy to help with this project if you add me to contributors on github and npm. I already help maintain a few other projects.

AriaMinaei commented 10 years ago

Hey Alex, the fix is great! Especially now that you don't have to delay everything for the next tick.

And it's very kind of you to offer help :) I'll add you to the project on Github and npm.

AriaMinaei commented 10 years ago

Done, please confirm.

alexgorbatchev commented 10 years ago

Got it, thanks! I've got a couple of suggestions that I want to run by you:

  1. Since this patch changes how pretty-error functions at the core, i feel a major version should change in the next release not to break existing integrations
  2. I maintain quite a few coffeescript packages and work at a shop where we use coffeescript exclusively. Would you be open to a minor code reorganization to shift towards more "common" approach? This would involve:
    • moving files a round a bit
    • getting rid of empty lines (this is a very unconventional formatting style). I recommend making a shift towards more conventional style to invite more participation from others.

What do you think?

AriaMinaei commented 10 years ago

I'm very sorry that I kept you waiting, Alex. I should have answered sooner, so, my apologies :)

About your suggestions:

  1. Agreed.
  2. Yeah, I guess it's not necessary or useful to do everything the "uncommon" way :)
    1. Yes, we came up with this kind of directory structure for projects that included too many different pieces of code in different languages. It makes sense for those projects, but it's not very useful here.
    2. I'm okay with removing the empty lines in this project. I see that it is an unfamiliar style. We actually chose this style after seeing how much it helps reading and understanding others' code. But it is unconventional and we can try a different style at least in this project.
alexgorbatchev commented 10 years ago

done, check it out :) i also updated tests to mocha...

one thing that bothers me about some of the tests is that they just console.log stuff out without checking... this isn't a valid test because there isn't a way to even break it.

AriaMinaei commented 10 years ago

Everything looks great (^_^)b

Yeah, about the tests, for now we can write tests that make sure PrettyError parses different error objects correctly, and we can also test the renderObject.

What do you think?