search

stevan / promises-perl

An implementation of Promises in Perl
31 stars 29 forks source link

stab at warnings on unhandled rejects #37

Closed ruz closed 7 years ago

ruz commented 9 years ago

Hi,

This is a stab to check ground. I often find myself spending some time looking for exception I forgot to handle. Mostly during prototyping and because of mistake (unexpected runtime exception) in a resolve callback. I find such a warning very helpful.

Do you see place in the master for something like this? If not I will just monkey patch.

stevan commented 9 years ago

So I am not against this idea, but I would like to understand some of the particulars of the patch.

First, (and this may be a stupid question) why do you have to warn in DESTROY and not when you detect the actual situation in _notify. I am sure you have a good reason, just easier if you explain the rationale. I ask this because DESTROY has a non-trivial overhead so while it is okay in development, I would like to have it not there in production (which can be accomplished in a number of ways).

Also, as I am sure you realize, introducing a new dependency (Data::Dumper::Concise) just for debugging is not ideal.

But yeah, in the end I am not against this, I always worried that leaving off the second handler was not so great, but we were following the spec so ... well we followed the spec.

stevan commented 9 years ago

Oh, also we would need to move the tttt.pl into a proper test.

ruz commented 9 years ago

a deferred can be rejected before you have any handlers on it, so you can not warn right away during _notify.

I don't remember if any dumpers are shipped with perl5 distribution, do you? If you want to inject DESTROY only when debug is activated then may be it's much easier to make this a separate distribution with dependency on a dumper. What do you think?

tttt.pl is just for demo.

stevan commented 9 years ago

I think this is useful for Promises itself, no need to make another distro.

Data::Dumper should be in the core and you should be able to get similar behavior to Data::Dumper::Concise with it using the various settings.

I think the right way to handle this is to add a DEBUG constant (perhaps set via some PROMISES_DEBUG env variable or something) and have that guard the setting of the slot in _notify and then guard a dynamic creation of a DESTROY method (should be easily doable with a glob assignment (*DESTROY = sub { ... } if DEBUG basically). Also, it would be nice if the slot (handled_reject) you store the boolean in was more "private", so maybe __handled_reject or something.

Then basically just make tttt.pl into a proper test to verify the behavior, etc.

How does that sound?