Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
correct labels for dicts #50
base: master
Are you sure you want to change the base?
correct labels for dicts #50
Changes from all commits
a931810
bcbc763
1d921d5
34b5072
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So AFAIU
neighbours
isedges
without duplicates, andcounts
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for a comment if we can add
more_itertools
:https://more-itertools.readthedocs.io/en/stable/api.html#more_itertools.unique_everseen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of lines down from here there's a
del neighbours
, and I think you might want to adddel 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I figured it wasn't worth it, could wrap this whole thing in a
def
so everything is automatically deletedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for adding
max-line-length = 80
to setup.cfg's flake8 section to avoid this line too long error.