search

twisted / ampoule

Process pool for Twisted, using AMP
Other
12 stars 19 forks source link

Replace all uses of twisted.python.log with twisted.logger. #29

Closed ldanielburr closed 5 years ago

ldanielburr commented 5 years ago

Just getting this PR started so that I don't lose motivation; all tests passing.

ldanielburr commented 5 years ago

Sorry, I should mention that this PR address #22.

ldanielburr commented 5 years ago

@glyph I've made one of the requested changes, and solicited feedback on the other requested change. Regarding your comment about "adding a test or two to extract some structure": can you elucidate a bit? I know Logger is all about emitting events that can be formatted as text or json or what-have-you, but I'm very foggy on how to go about testing "structure". Are you referring to the raw events being emitted, or the formatted representation of those events?

glyph commented 5 years ago

Are you referring to the raw events being emitted, or the formatted representation of those events?

The event dictionaries being emitted. Such a test would add a log tap for the duration of the test, and assert about some stuff in the event dicts logged; hopefully this is a lightweight thing to do, but if it's at all hard let's skip it and come back to it later - there's no direct tests for this as it stands and I don't want the perfect to be the enemy of the good here.

ldanielburr commented 5 years ago

@glyph regarding unit-tests for the new logger: I can't get my head around it, sorry. It seems to me that twisted.logger's unit-tests must already be testing the emission of events so what additional surety would be provided here?

glyph commented 5 years ago

Hmm, we probably need to do a release for this...

ldanielburr commented 5 years ago

@glyph, please note https://twistedmatrix.com/trac/ticket/9680, which is the reason for the large number of logger instances created when making a logger a class member. If this twisted.logger ticket could be resolved, class-based loggers might become a better option than what I ended up doing here.

glyph commented 5 years ago

Thanks for the reference. I'm not actually sure that module-level loggers are all that bad of an idea, honestly… the main reason to use instance-level ones is for convenience logging fields off the instance; module-level namespace granularity seems sufficient for most cases.