Conversation
graingert
force-pushed
the
patch-2
branch
3 times, most recently
from
94428ef to
9e2b378
Compare
|
@mgedmin can I get a review |
mgedmin
left a comment
mgedmin
left a comment
There was a problem hiding this comment.
Thank you, this is excellent!
I have only a couple of minor comments that I'll leave to your discretion. Also, would you like to add a changelog entry for this fix?
(Apologies for a late reply, I was on vacation and was pretending computers didn't exist.)
| edges = edge_func(target) | ||
| counts = collections.Counter(id(v) for v in edges) | ||
| neighbours = list({id(v): v for v in edges}.values()) | ||
| del edges |
There was a problem hiding this comment.
So AFAIU neighbours is edges without duplicates, and counts is the count of duplicates. This took me a while to understand, maybe a comment would be helpful?
There was a problem hiding this comment.
no need for a comment if we can add more_itertools:
https://more-itertools.readthedocs.io/en/stable/api.html#more_itertools.unique_everseen
neighbours = more_itertools.unique_everseen(edges, key=id)
There was a problem hiding this comment.
I'd rather not; I like that objgraph is a single file library with no external dependencies I can copy over into any random location on my python path and start using immediately.
objgraph.py
Outdated
| return [' [label="f_locals",weight=10]'] | ||
| if target is source.f_globals: | ||
| return ' [label="f_globals",weight=10]' | ||
| return [' [label="f_globals",weight=10]'] |
There was a problem hiding this comment.
I wonder if there are any frames where frame.f_locals is frame.f_globals and we'd have the same problem with two arrows having the same label?
I wonder if it wouldn't be a good idea to use yield here for easier handling of multiple return values.
There was a problem hiding this comment.
I tried using yield but it breaks the control flow around return
There was a problem hiding this comment.
oh I see yes that might be much better
There was a problem hiding this comment.
It would require replacing all return statements with yield, and then adding return with no arguments for short-circuiting the control flow (or rewriting the series of top-level 'if/if/if' into 'if/elif/elif').
Not worth it. When I first suggested yield, I thought there would be more places with possible duplicate labels, but it looks like just this one place.
objgraph.py
Outdated
| ' [label="%s",weight=10]' % _quote(k) | ||
| for k in dir(source) | ||
| if target is getattr(source, k) | ||
| ] |
tests.py
Outdated
| @skipIf( | ||
| sys.version_info < (3, 6), | ||
| "Python < 3.6 dicts have random iteration order", | ||
| ) |
There was a problem hiding this comment.
I'm tempted to do a quick hack with
lines = output.readlines()
self.assertEqual(
''.join(lines[:5] + sorted(lines[5:-3]) + lines[-3:]),
textwrap.dedent(...)
)so this test can run on all Python versions.
There was a problem hiding this comment.
but the test looks so beautiful without it
There was a problem hiding this comment.
The rest of the test will continue to look beautiful, and the sorted(lines[5:-3]) will inspire awe in the unwary reader. ;)
There was a problem hiding this comment.
Python 3.5 reached End Of Life recently, so the point is moot.
| _obj_node_id(tgtnode), elabel)) | ||
| if id(source) not in depth: | ||
| depth[id(source)] = tdepth + 1 | ||
| queue.append(source) |
There was a problem hiding this comment.
A couple of lines down from here there's a del neighbours, and I think you might want to add del counts in there.
It probably doesn't matter, counts holds only ints, and I doubt anyone expects specific refcounts for those, and also they won't be causing problems with memory usage.
There was a problem hiding this comment.
yeah I figured it wasn't worth it, could wrap this whole thing in a def so everything is automatically deleted
objgraph.py
Outdated
| zip_longest = itertools.izip_longest | ||
| except AttributeError: # pragma: PY3 | ||
| # Python 3.x compatibility | ||
| zip_longest = itertools.zip_longest |
There was a problem hiding this comment.
can we use six.moves.zip_longest instead?
There was a problem hiding this comment.
I don't want to introduce non-optional dependencies outside Python's stdlib yet.
should detect cases where frame.f_locals is frame.f_globals
| if _isinstance(k, basestring) and _is_identifier(k): | ||
| yield ' [label="%s",weight=2]' % _quote(k) | ||
| else: | ||
| yield ' [label="%s"]' % _quote(tn(k) + "\n" + _safe_repr(k)) |
There was a problem hiding this comment.
+1 for adding max-line-length = 80 to setup.cfg's flake8 section to avoid this line too long error.
Co-authored-by: Marius Gedminas <marius@gedmin.as>
| elabel = _edge_label(srcnode, tgtnode, shortnames) | ||
| f.write(' %s -> %s%s;\n' % (_obj_node_id(srcnode), | ||
| _obj_node_id(tgtnode), elabel)) | ||
| for elabel, _ in itertools.zip_longest( |
There was a problem hiding this comment.
This doesn't work on Python 2.7.
I'd have to stop and think whether I want to drop Python 2.7 compatibility at this point.
2 participants
Fixes #49