MOD:add parameter 'filter' to some method and show lambda more friendly by xybaby · Pull Request #33 · mgedmin/objgraph

xybaby · 2017-12-17T13:02:38Z

add parameter 'filter' to some function(mainly to typestats), which is very helpful with complex project
show lambda function with filename and lineno, which is more meaningful

mgedmin

Thank you for your contribution! In order for it to be included, it needs just a few little touches:

a couple of lines at the top of the CHANGES.rst file: one mentioning the new function argument and one mentioning the nicer lambda representation
documentation for the new function argument in the docstrings of functions that take it
two unit tests for the new code.

It would be ideal if you could update your PR to do at least some of the above. Please let me know if you are unable or unwilling, and I'll try to find the time to do them myself.

mgedmin · 2017-12-18T10:46:40Z



-def typestats(objects=None, shortnames=True):
+def typestats(objects=None, shortnames=True, filter=None):


This new parameter needs documentation in the docstring, and a .. versionchanged: ... directive.

Thanks very much, and i have do some fix with your advise, please check again

mgedmin · 2017-12-18T10:47:08Z



-def most_common_types(limit=10, objects=None, shortnames=True):
+def most_common_types(limit=10, objects=None, shortnames=True, filter=None):


Likewise.

EDIT: I mean this needs a docstring update. No unit test necessary.

mgedmin · 2017-12-18T10:47:27Z

        shortnames=True,
-        file=None):
+        file=None,
+        filter=None):


mgedmin · 2017-12-18T10:48:13Z



-def show_growth(limit=10, peak_stats={}, shortnames=True, file=None):
+def show_growth(limit=10, peak_stats={}, shortnames=True, file=None, filter=None):


mgedmin · 2017-12-18T10:50:53Z

        else:
            return name
+    if _isinstance(obj, types.LambdaType):
+        return 'lambda: %s:%s' % (os.path.basename(obj.func_code.co_filename), obj.func_code.co_firstlineno)


This is nice.

What is missing is a unit test. Currently objgraph has 100% test coverage, and I want to maintain that.
There are a bunch of copy-able examples called test_short_repr_... in tests.py. I suggest using assertRegex so you don't have to hardcode the line number of the lambda.

mgedmin · 2017-12-18T10:55:16Z

        stats = {}
        for o in objects:
+            if filter and not filter(o):
+                continue


This change needs a unit test. See TypestatsTest in tests.py. I suggest defining a new class, creating a couple of instances, then asserting that typestats()[name-of-that-class] returns 2 without a filter, and 1 with a filter that rejects one of the instances (e.g. by attribute value).

mgedmin

Let's fix the surprising test failure on Python 3.4/Windows and it looks good to merge!

mgedmin · 2017-12-19T14:00:29Z

+
+3.1.3 (unreleased)(2017-12-19)
+------------------
+


Thank you for these! But you accidentally duplicated the 3.1.3 version header. Also, the new entries should be below the header (replace the existing "- Nothing changed yet." text). And there's no need to add a date -- I'll add it when I make the release.

mgedmin · 2017-12-19T14:03:57Z

    will be lumped together if ``shortnames`` is True.

+    Use ``filter`` (a predicate) to remove undesired objects.
+


Very good, only I'd also like to clarify whether the objects for which filter() returns True are included or excluded.

Perhaps something like

If ``filter` is specified, it should be a function taking one argument and returning a boolean. Objects for which ``filter(obj)`` returns ``False`` will be ignored.

?

mgedmin · 2017-12-19T14:04:21Z


+class TypestatsFilterArguTest(GarbageCollectedMixin, unittest.TestCase):
+    """Tests for the typestats function, especially for augument
+    ``fiter`` which is added at version 3.1.3"""


Typo: fiter -> filter.

mgedmin · 2017-12-19T14:05:34Z

+    def test_short_repr_lambda(self):
+        f = lambda x: x  # noqa
+        lambda_lineno = sys._getframe().f_lineno - 1
+        self.assertEqual('lambda: tests.py:%s' % lambda_lineno,


That is a clever way of getting the line number!

mgedmin · 2017-12-19T14:09:17Z

+        y.magic_attr = False
+        stats = objgraph.typestats(
+            shortnames=False,
+            filter=lambda e: hasattr(e, 'magic_attr') and e.magic_attr)


This fails on Python 3.4 on Windows because, apparently, hasattr(ctypes.cdll, 'magic_attr') tries to load a magic_attr.dll and raises an OSError. Amazing.

I suggest replacing the hasattr check with an isinstance(e, MyClass) check.

This is a magic error, i had run the test on my computer,(windows, python3.4) and no error .
Thank for your advice, it works!

mgedmin

Thank you very much!

mgedmin · 2017-12-20T08:50:04Z

Oops, a regression: this makes regular functions show as lambdas.

I'll fix it in master.

MOD:add parameter 'filter' to some method and show lambda more friendly

fa666bb

mgedmin requested changes Dec 18, 2017

View reviewed changes

mgedmin added the enhancement label Dec 18, 2017

xybaby added 3 commits December 19, 2017 10:26

MOD: add change log, unittest case, docstring

e5f8848

MOD: Compatibility with python3.x

e72c7f1

MOD: Compatibility with pep8

77ec760

mgedmin requested changes Dec 19, 2017

View reviewed changes

MOD:fix fail on python3.4 windows,and more clearly docstring

3647595

mgedmin approved these changes Dec 20, 2017

View reviewed changes

mgedmin merged commit 0bf74b2 into mgedmin:master Dec 20, 2017



		def typestats(objects=None, shortnames=True):
		def typestats(objects=None, shortnames=True, filter=None):



		def most_common_types(limit=10, objects=None, shortnames=True):
		def most_common_types(limit=10, objects=None, shortnames=True, filter=None):



		def show_growth(limit=10, peak_stats={}, shortnames=True, file=None):
		def show_growth(limit=10, peak_stats={}, shortnames=True, file=None, filter=None):

		will be lumped together if ``shortnames`` is True.

		Use ``filter`` (a predicate) to remove undesired objects.

Uh oh!

Conversation

xybaby commented Dec 17, 2017

Uh oh!

mgedmin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgedmin Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgedmin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xybaby Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgedmin left a comment

Choose a reason for hiding this comment

Uh oh!

mgedmin commented Dec 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mgedmin Dec 18, 2017 •

edited

Loading

xybaby Dec 20, 2017 •

edited

Loading