Broken documentation/feature #50

Merged
merged 1 commit into from Oct 7, 2013

Conversation

Projects
None yet
2 participants
Contributor

mbr commented Mar 14, 2012

The specific example in the handler.BaseHandler docs does not work, see https://gist.github.com/2036228 for a test case. One issue is that the docs have confused True and False in the example. Fixing that, it stil does not work, however.

The "shortcut" in logbook/base.py:899 seems to be the issue:

            # if this is a blackhole handler, don't even try to
            # do further processing, stop right away.  Technically
            # speaking this is not 100% correct because if the handler
            # is bubbling we shouldn't apply this logic, but then we
            # won't enter this branch anyways.  The result is that a
            # bubbling blackhole handler will never have this shortcut
            # applied and do the heavy init at one point.  This is fine
            # however because a bubbling blackhole handler is not very
            # useful in general.
            if handler.blackhole:
                break

This is the only application of the blackhole-feature that I could find and it breaks it (disabling the check causes everything to work as expected).

Please have a look at the very simple test case I've provided.

Owner

mbr commented on a05f64e Mar 14, 2012

Note that this will fix the docs, but the example will still not work. See https://gist.github.com/2036228 for an example.

@vmalloc vmalloc added a commit that referenced this pull request Oct 7, 2013

@vmalloc vmalloc Merge pull request #50 from mbr/master
Fix documentation for handlers.Handler
830bcd5

@vmalloc vmalloc merged commit 830bcd5 into getlogbook:master Oct 7, 2013

Contributor

mbr commented Apr 20, 2015

Still not fixed as of now, I'm afraid. At this point I am not sure what the intention is. See https://gist.github.com/mbr/6afa28c8b9f0b71d4cf4 for a really simple example:

from logbook import NullHandler, critical

handler = NullHandler(bubble=True)
handler.push_application()

critical('sample message, should bubble')

The only fix is adding a handler.blackhole = False before logging the message. If that is working as intended, I'd love to see the docs updated, otherwise the implementation fixed. I'll even do it, just let me know which way is correct!

The bug is annoying me to no end because I run into it about once a year, after I've forgotten about it =).

Collaborator

vmalloc commented Apr 20, 2015

I touched that area not too long ago and from what I could tell blackhole is there only to serve NullHandler.

Just a quick question: how do you expect the code you attached to behave?

Contributor

mbr commented Apr 20, 2015

As in the docs. The whole blackhole attribute looks more like a helper for the implementation. I'd expect the NullHandler to do nothing with the exception and then bubble it upwards to the default StderrHandler.

Collaborator

vmalloc commented Apr 20, 2015

@mbr what you wrote isn't an exception - it is a log of level 'critical'. Do you mean you expect it to bubble it because the level on the NullHandler is lower than critical?

Collaborator

vmalloc commented Apr 20, 2015

And if you can, please add a link to the contradicting or wrong area of the docs.

Contributor

mbr commented Apr 20, 2015

@vmalloc:

what you wrote isn't an exception - it is a log of level 'critical'. Do you mean you expect it to bubble it because the level on the NullHandler is lower than critical?

Yes. critical is there purely for illustrative purposes. You can substitate any loglevel and it will behave the same for all intents and purposes. I made a mistake above, "exception" should actually read "message". Really sorry about that one =(.

What it should do (and does, once you set blackhole = False) is bubble up anything sent to the NullHandler(). The StderrHandler uses level 0 by default so the message gets displayed.

And if you can, please add a link to the contradicting or wrong area of the docs.

I've linked and quoted the relevant section in the gist (the one from today, not the one from 3 years ago). It's from the docstring at https://github.com/mitsuhiko/logbook/blob/master/logbook/handlers.py#L126

@vmalloc vmalloc added a commit that referenced this pull request May 17, 2015

@vmalloc vmalloc Clarify documentation (relates to #50) 1059fce
Collaborator

vmalloc commented May 17, 2015

I pushed a small fix to that area of the documentation, not feeling comfortable enough to change the semantics of NullHandler bubbling at this point. Please see if the change makes sense to you and let me know otherwise.

Contributor

mbr commented May 17, 2015

It's great because it takes out the confusion for people picking up the
library and wondering why they can't get it working. I would suggest you
add another example of using bubbling because a correct one is really
helpful and mention the non-standard behavior of the NullHandler.

Thank's a lot for the fix.

On Sun, May 17, 2015 at 9:35 AM, Rotem Yaari notifications@github.com
wrote:

I pushed a small fix to that area of the documentation, not feeling
comfortable enough to change the semantics of NullHandler bubbling at this
point. Please see if the change makes sense to you and let me know
otherwise.


Reply to this email directly or view it on GitHub
mitsuhiko#50 (comment).

Collaborator

vmalloc commented May 17, 2015

No problem. Thanks for helping out and sorry for the long delay in getting to this :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment