itamarst / eliot

Eliot: the logging system that tells you *why* it happened
https://eliot.readthedocs.io
Apache License 2.0
1.11k stars 66 forks source link

The write_failure documentation gives a misleading usage example #371

Closed exarkun closed 5 years ago

exarkun commented 5 years ago

The docstring gives the example:

        d = dostuff()
        d.addCallback(process)
        # Final error handler.
        d.addErrback(writeFailure, logger, "myapp:subsystem")

Of course dostuff could be anything and may lead to the correct behavior but real-world, idiomatic code that does the correct thing is more likely to be:

        d = DeferredContext(dostuff())
        d.addCallback(process)
        # Final error handler.
        d.addErrback(writeFailure, logger, "myapp:subsystem")

I think it should be clear to most readers that the DeferredContext is required for process to be run in the correct action context. However, since this is the documentation for writeFailure, it probably makes sense to take care to point out that the DeferredContext is required for the failure to end up logged as part of the correct action (which makes plenty of sense but it wasn't obvious to me, reading the example, and I had to try it out to convince myself there wasn't some special behavior surrounding writeFailure that made the DeferredContext unnecessary).

This all assumes there is an active action. I think that's probably the common case, given the purpose of Eliot, but maybe it's worth also mentioning in the docstring, along with this change.