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

Support old-style class names. And support shortname logic properly everywhere #16

Merged
merged 3 commits into from Feb 25, 2015

Conversation

Projects
None yet
2 participants
@mikelambert
Copy link

mikelambert commented Feb 25, 2015

No description provided.

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Feb 25, 2015

Oh, my good point.

The only thing that worries me is that some proxies out there masquerade their class. I'd rather use type(obj) but fall back to obj.__class__.__name__ if the type is types.InstanceType.

Also, this ought to have a test (one that's skipped on Python 3 since Python 3 has no old-style classes).

Only use our old-style-class support in the cases where it's actually…
… a <type 'instance'>. Also add some tests to verify this works.
@mikelambert

This comment has been minimized.

Copy link

mikelambert commented Feb 25, 2015

Not sure if github re-notifies you when I add more changes to the pull request. But thanks for the feedback, I've made the changes you've requested.

@mgedmin

This comment has been minimized.

Copy link

mgedmin commented on tests.py in 34a1b76 Feb 25, 2015

Test for short_typename :)

This comment has been minimized.

Copy link
Owner

mikelambert replied Feb 25, 2015

Ok fixed, along with the other "Test for long_typename" misnomers that had crept in. :)

@mgedmin

This comment has been minimized.

Copy link

mgedmin commented on objgraph.py in 34a1b76 Feb 25, 2015

This fails on Python 3, where types.InstanceType does not exist.

Suggestion:

try:
    from types import InstanceType
except ImportError:  # Python 3
    InstanceType = None

This comment has been minimized.

Copy link
Owner

mikelambert replied Feb 25, 2015

Ahh good point (and good idea), fixed. Sorry, I'm not a python3-er so don't have a proper test environment set up for that over here.

This comment has been minimized.

Copy link

mgedmin replied Feb 25, 2015

No problem, that's why I have Travis :-)

As for myself, I find detox allows me to painlessly support multiple Python versions, once I get them all installed (and here Ubuntu PPAs help a lot. I've also heard great things about pyenv, but haven't tried it myself).

@mgedmin

This comment has been minimized.

Copy link
Owner

mgedmin commented Feb 25, 2015

Not sure if github re-notifies you when I add more changes to the pull request

It doesn't. Thanks for the ping!

@mikelambert

This comment has been minimized.

Copy link

mikelambert commented Feb 25, 2015

Ok cool, re-ping.

@@ -249,6 +249,25 @@ def doctest_short_repr_unbound_method():
"""

This comment has been minimized.

@mgedmin

mgedmin Feb 25, 2015

Owner

Nitpick: PEP-8 specifies two blank lines between top-level functions.

I didn't catch this earlier so instead of making you jump through hoops now I think I'll just merge it and fix it later.

mgedmin added a commit that referenced this pull request Feb 25, 2015

Merge pull request #16 from mikelambert/master
Support old-style class names. And support shortname logic properly everywhere

@mgedmin mgedmin merged commit 9ff872a into mgedmin:master Feb 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

mgedmin added a commit that referenced this pull request Feb 25, 2015

pcostell added a commit to pcostell/objgraph that referenced this pull request Feb 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment