Code-level simplifications to tracing#226
Conversation
The way _TracedHookExecution is implemented, it takes the PluginManager instance, which creates a cyclic dependency between the manager.py and _tracing.py modules. It also mutates internal variables of PluginManager. This makes the code harder to understand. Just inlining it makes things mostly straightforward. This commit also removes an assert which prevented a trace from being added if there is already an active one. I don't see any reason why it was done; seems like a legitimate thing to do and should work just fine.
This function is undocumented and unused internally. pytest used to use it in a test case, but hasn't done so since 2010: pytest-dev/pytest@b3628da#diff-5fd183e022c6cb9ca47f6c1ffc09eadeL274 A code search on GitHub only finds it inside copies of pluggy itself.
They are only directly used internally and not documented, Makes it easier to see what is exposed.
It was only used as a kind of namespacing, but it makes things harder to follow. Since TagTracerSub doesn't have any side-effects, avoid storing it and make its use entirely localized. pytest, devpi and tox don't use PluginManager.hook._trace.
If a KeyError is raised by the user-supplied processor function, it should propagate.
The key is a "path" (tuple) of tags, not a single tag.
Codecov Report
@@ Coverage Diff @@
## master #226 +/- ##
==========================================
+ Coverage 93.08% 93.86% +0.78%
==========================================
Files 14 9 -5
Lines 1677 1109 -568
Branches 117 19 -98
==========================================
- Hits 1561 1041 -520
+ Misses 101 66 -35
+ Partials 15 2 -13
Continue to review full report at Codecov.
|
bluetech
left a comment
There was a problem hiding this comment.
Thanks for the review @nicoddemus - left a reply.
|
Looks great, thanks again. @goodboy would you like to review as well? |
| self._tag2proc = {} | ||
| self.writer = None | ||
| self._tags2proc = {} | ||
| self._writer = None |
There was a problem hiding this comment.
as far as i can tell the "writer" is a "print" style function we ight be able to simplify
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
looks good, i wonder if setwriter could be deprecated in davour of naming _writertoprinter`
as far as i can tell, all writer is, is a optional printer
overall this is fine to merge as is, good work
This is a bit more readable.
|
@goodboy, absolutely no rush here, but would you like for us to wait here so you can review this, or are you OK with us merging this? 😁 |
|
@nicoddemus so sorry! |
|
No worries at all, take your time. 👍 |
|
Updated with @goodboy's suggestion. |
|
@bluetech thanks again for the help! More eyes on this puppy is great to see! |
A few minor changes relating to the tracing stuff. Please see the commits.