bpo-6691: Support for nested classes and functions in pyclbr #2503

csabella · 2017-06-30T10:45:59Z

Based on the patch by Guilherme Polo.

terryjreedy

After the patch, the biggest flaw of the module, in my opinion, will be that is discards the line order of statements. If a collections.OrderedDict is forward compatible with dicts, I would like to make the replacement. I want to ask Raymond H about this.

Cheryl, there are a couple of questions about the isinstance checks that might be related to the test failure and fix you mentioned.

terryjreedy · 2017-07-01T03:37:45Z

Doc/library/pyclbr.rst

@@ -39,78 +39,77 @@ modules.
   path.


I presume that the opening paragraph should be revised. 'top-level' removed. I don't know what a 'traditional three-pane class browser' is, The addition of functions makes this a module browser.

readmodule_ex: since there is not another function added, I presume that the return of readmodule_ex has been expanded so that 'top-level' is obsolete here too. I will look at the actual behavior before editing.

I am guessing that I cannot hide this as outdated because there was no subsequent change at this location. I will try approving and then making a new review.

terryjreedy · 2017-07-01T03:42:14Z

Doc/library/pyclbr.rst

+--------------
+The class :class:`Object` is the base class for the classes
+:class:`Class` and :class:`Function`. It provides the following
+data members:


An added in 3.7 note is needed. I am not sure if it goes here or after attributes. In any case, most attributes were already common to Class and Function.

Addition notes looks good to me, but I am not clear on the rules. See comment below about core-mentorship question.

Approval did not collapse these.

terryjreedy · 2017-07-01T03:42:57Z

Doc/library/pyclbr.rst


-.. attribute:: Class.lineno
+   The parent of this object, if any.


New attribute added in 3.7

Comment misplaced. Actual new attribute now tagged.

terryjreedy · 2017-07-01T03:43:28Z

Doc/library/pyclbr.rst


-.. _pyclbr-function-objects:
+   A dictionary mapping object names to the objects that are defined inside the
+   namespace created by the current object.


New attribute added in 3.7. By inheritance, added in 3.7 to Class and Function. As I say below, this new attribute should be 'children'.

terryjreedy · 2017-07-01T03:45:42Z

Doc/library/pyclbr.rst

+   classes of the class being described.  Classes which are named as
+   superclasses but which are not discoverable by :func:`readmodule` are
+   listed as a string with the class name instead of as :class:`Class`
+   objects.


This and 'methods' below, with the 'old' attributes in Object, constitute the old set of attributes for Class.

No specific fix needed.

terryjreedy · 2017-07-01T05:39:32Z

Lib/pyclbr.py

-                                               fname, lineno)
-                stack.append((None, thisindent)) # Marker for nested fns
+                    cur_func = Function(fullmodule, meth_name, fname, lineno)
+                    dict[meth_name] = cur_func


140 # Initialize the dict for this module's contents
141 dict = {}
I dislike this and would like to fix it. 'obs'? If '.objects' is renamed 'children', we could rename the master dict 'objects'. Or to be more specific, 'heads'. Since each top-level object is parentless and the head of a tree in the forest. 'tree' or 'trees' and 'forest' are also possibilities. "tree['f'] = Function(..,'f',...)" makes some sense.

dict changed to tree

terryjreedy · 2017-07-01T06:04:51Z

Lib/pyclbr.py

 def _main():
    # Main program for testing.
    import os
-    from operator import itemgetter
    mod = sys.argv[1]


sys.argv[1] may not exist if run from command line and generally will not if run from an editor. To default it to running on itself:
try:
mod = sys.argv[1]
except:
mod = file

Awesome! I was calling this a lot when testing.

And I did so thrice more. Fixed.

Thnks. Im still lost

terryjreedy · 2017-07-01T06:18:45Z

Lib/test/test_pyclbr.py

@@ -150,6 +154,121 @@ def test_decorators(self):
        #
        self.checkModule('test.pyclbr_input', ignore=['om'])

+    def test_nested(self):
+
+        def clbr_from_tuple(t, store, parent=None, lineno=1):


tup instead of t

terryjreedy · 2017-07-01T06:24:48Z

Lib/test/test_pyclbr.py

@@ -2,10 +2,14 @@
   Test cases for pyclbr.py
   Nick Mathewson
 '''
+


The unpatched code, which I cannot directly comment on, has multiple print calls. I believe that production test code should not have these. Will ask on core-mentorship.

In any case, another issue.

terryjreedy · 2017-07-01T06:54:06Z

Lib/test/test_pyclbr.py

+        def pickp(name):
+            return pclass if name.startswith('class') else pfunc
+
+        # Create a module for storing the Python code.


_readmodule is about 200 lines. The first 58 lines turn module name, path, and package in source and fname. The rest turn source and fname into the dict to be returned. The first part is tested elsewhere. If we sensibly split is into private functions as indicated above, we could drop all this bother of turning source into a temp file and just pass it to the 2nd new function.

Split done. Test not yet simplified because _create_tree needs 5 arguments, not 2. I would still like to simplify by new tests, but since they are working, this could wait for a separate pyclbr tests issue.

terryjreedy · 2017-07-01T16:06:32Z

Some of the changes I suggested are outside the scope of this issue and should be left for another. Using DefaultDict is one issue. Removing print calls from tests is another. I think splitting the private function into two to simplify the test of the new feature can be included, but as a separate commit.

csabella · 2017-07-01T22:20:22Z

I made a separate commit for splitting _readmodule into two functions. It's only 5 lines, including whitespace and docstring. It didn't pass the tests at first and I've been debugging it for hours. It ended up that the calls back to _readmodule needed path and inpackage and I didn't include those at first. My question -- how could I have seen that sooner instead of spending hours debugging? It gave my a NameError once I figured out what to watch, but the new function didn't have those names defined, so I was surprised it failed silently.

Thanks!

terryjreedy · 2017-07-02T22:03:30Z

I believe _Object should be private, if not removed. I asked on core-mentorship about the current added/changed notes as well as what is allowed in the doc if Object is removed.
EDIT: Object has a common method as well as data attributes, so I think to keep it as _Object.

terryjreedy · 2017-07-02T23:35:38Z

In a previous version of this comment, I said
"readmodule() needs to be revised to only return toplevel classes, as was the case before this patch."
The need for revision is wrong as all nested Objects are in new .children attributes. Old code should never see the addition.

It is still true that there is no test of readmodule, and that one might be added in a new test revision issue. With a test, the implementation could be reduced to a dict comprehension.

terryjreedy · 2017-07-03T00:37:09Z

Find in Files says neither _getname nor _getnamelist appear in test_pyclbr. Adding some would be another issue. Without knowing in detail what tokenizer emits, I am not sure they will handle legal multiline imports like
from tkinter import (Tk as #nasty split
k, mainloop as ml)

terryjreedy · 2017-07-03T01:11:12Z

I assumed that any missing parameters would immediately result in NameErrors. If the test failed because of a missing parameter, and did not raise NameError, I would be surprised and puzzled. Looking at the test, one problem I see is that test_others() does multiple cm tests in sequence, so first to fail stops testing of the rest. If different files exercise different pathways with different needed parameters, it would take several runs to get them all. I am sorry that this was much more troublesome that I expected.

This function should be revised to use subtests (self.subTest) so all run regardless of failure. This is at least the fourth thing to fix in this file. Prints, temporary file, and missing tests are another.

terryjreedy · 2017-07-03T02:35:39Z

After review, I think the only thing left for this issue is to make Object private. If I get no response on core mentorship, I will copy the current redundant doc format, but put Function first and note on Class that it has 2 more. The doc should also mention that Class.methods is redundant with Class.children in that the former lists the Functions in the latter. It is only kept for back-compatibility.

csabella · 2017-07-03T13:01:07Z

I am sorry that this was much more troublesome that I expected.

This wasn't a problem at all. Every chance I get to debug and track something down is more learning for me, so it's all good. I apologize if it sounded like I was complaining. I just brought it up because I was surprised I didn't get a warning while running the tests and to find if there were any tricks (or tools) that would have helped me reach the endpoint sooner.

terryjreedy

I changed my mind about the new test. It is so opaque that I trust it less than the code. The function that creates code I cannot see from a tuple structure and the the custom equality function that compares the nested output to nodes created from the tuple structure need tests themselves ;-).

Transparent would be a chunk of code that is fed to readmodule_ex and a constructed dict that is obviously correct for the code, which is compared to the output of readmodule_ex. Python already has tested dict equality comparison.

The tricky part of the construction is the double linkage, which is completed with _add_child. This new method should be tested (easy).

terryjreedy · 2017-07-03T15:26:56Z

Doc/library/pyclbr.rst


-Class Objects
-------------
+Object Objects


I want to make Object private and delete this part of the doc.

terryjreedy · 2017-07-03T15:27:44Z

Lib/pyclbr.py

-    '''Class to represent a Python class.'''
-    def __init__(self, module, name, super, file, lineno):
+
+class Object:


_Object here and below.

I think I need to figure out how to pull in your commit if you would like me to make these changes.

The docstrings and comments are already part of your branch, and visible when I hit [files] at the top. I have already done the _Object changes, the _create_tree docstring and setup for the revised test. I am working on the hard part -- a revised comparison function. I hope to push all within an hour.

terryjreedy · 2017-07-03T15:51:25Z

Lib/pyclbr.py


+def _create_tree(fullmodule, path, fname, source, tree, inpackage):


This needs docstring with explaination of parameters, which is needed to make direct call in test.

csabella · 2017-07-04T11:42:43Z

Thanks for making all those changes. I know you said it needs more, but it looks awesome!

terryjreedy · 2017-07-04T18:38:36Z

I think I did what I said was essential. Anyway, there were two conceptual improvements.

Expand the helper from 'create nested object' to 'add nested object'. This eliminated having to duplicate code when creating 'expected' in the test.

Added now: I believe the helpers should not just call .add_child, but should be incorporated into it, and the Class override.

Testing equality of the dicts failed because the default equality for Objects is by identity. Adding .eq failed because the parent-child-parent led to infinite recursion. Fiddling a bit with .eq did not work together. I then realized that I needed to compare nodes with data and children. So I paired None with its conceptual children to make a conceptual node. A top down equality test that tested each pair of nodes just once was then pretty easy to write.

Side node: general tree equality is hard because it is not initially known which child to pair with which. Here, the children for each node are uniquely labelled and the labels must match.

Added now. This is still clumsy. A possible solution: rename _Object as _Node. Replace None as the mod-level parent with top, an instance of _Node. Prime the _create_tree pump by initializing stack with [(top, -1)]. Since top is never popped, stack is never empty and we dump the stack test. Add all Function and Class node with (stack[-1][0]).add_child.

The creation of 'expected' in the test start with top = _Node(...) and all the addition would be with something.add_child. The compare function would have two parameters, node1 and node2, and would be .eq of _Node (where it could be tested).

_Node would then need one more method, .export_children. It would change the parent of all the children to None and then return the dict to be returned to users. Just where to do this would be a detail to be worked out after checking the tests.

I am not going to do this, unless revision is needed for IDLE. I would rather work on applying the lesson of matching code to concepts to IDLE.

…2503)" This reverts commit 246ff3b.

* Revert "bpo-30854: Fix compile error when --without-threads (#2581)" This reverts commit 0c31163. * Revert "NEWS for 30777 (#2576)" This reverts commit aaa917f. * Revert "bpo-21624: IDLE -- minor htest fixes (#2575)" This reverts commit 2000150. * Revert "bpo-30777: IDLE: configdialog - add docstrings and improve comments (#2440)" This reverts commit 7eb5883. * Revert "bpo-30319: socket.close() now ignores ECONNRESET (#2565)" This reverts commit 67e1478. * Revert "bpo-30789: Use a single memory block for co_extra. (#2555)" This reverts commit 378ebb6. * Revert "bpo-30845: Enhance test_concurrent_futures cleanup (#2564)" This reverts commit 3df9dec. * Revert "bpo-29293: multiprocessing.Condition.notify() lacks parameter `n` (#2480)" This reverts commit 4835041. * Revert "Remove outdated FOX from GUI FAQ (GH-2538)" This reverts commit d3ed287. * Revert "bpo-6691: Pyclbr now reports nested classes and functions. (#2503)" This reverts commit 246ff3b. * Revert "bpo-29464: Rename METH_FASTCALL to METH_FASTCALL|METH_KEYWORDS and make (#1955)" This reverts commit 6969eaf. * Revert "bpo-30832: Remove own implementation for thread-local storage (#2537)" This reverts commit aa0aa04. * Revert "bpo-30764: Fix regrtest --fail-env-changed --forever (#2536)" This reverts commit 5e87592. * Revert "bpo-30822: Deduplicate ZoneInfoTest classes in test_datetime. (#2534)" This reverts commit 34b5487. * Revert "bpo-30822: Fix testing of datetime module. (#2530)" This reverts commit 98b6bc3.

the-knights-who-say-ni added the CLA signed label Jun 30, 2017

terryjreedy requested changes Jul 1, 2017

View reviewed changes

csabella added 2 commits July 1, 2017 13:37

bpo-6691: Support for nested classes and functions in pyclbr

b94e281

bpo-6691: Support for nested classes and functions in pyclbr

a726dc1

csabella force-pushed the bpo6691 branch from e7c876b to a726dc1 Compare July 1, 2017 17:41

Split _readmodule into two functions

7105eed

Remove conditional for isinstance

bcc1760

Edit docstrings and comments.

83b10c7

terryjreedy approved these changes Jul 3, 2017

View reviewed changes

terryjreedy requested changes Jul 3, 2017

View reviewed changes

terryjreedy added 5 commits July 3, 2017 11:58

Merge remote-tracking branch 'upstream/master' into pr_2503

b9243d7

Revise doc

2c28520

Replace Object with _Object

082e469

Expand docstring for _create_tree

ed1b892

Expand helper functions; replace new tests.

259616c

terryjreedy merged commit 246ff3b into python:master Jul 4, 2017

csabella deleted the bpo6691 branch July 4, 2017 11:21

vstinner added a commit that referenced this pull request Jul 5, 2017

Revert "bpo-6691: Pyclbr now reports nested classes and functions. (#…

957a410

…2503)" This reverts commit 246ff3b.


		.. attribute:: Class.lineno
		The parent of this object, if any.


		def _create_tree(fullmodule, path, fname, source, tree, inpackage):

bpo-6691: Support for nested classes and functions in pyclbr #2503

bpo-6691: Support for nested classes and functions in pyclbr #2503

Conversation

csabella commented Jun 30, 2017

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy commented Jul 1, 2017

csabella commented Jul 1, 2017

terryjreedy commented Jul 2, 2017 • edited Loading

terryjreedy commented Jul 2, 2017 • edited Loading

terryjreedy commented Jul 3, 2017

terryjreedy commented Jul 3, 2017

terryjreedy commented Jul 3, 2017 • edited Loading

csabella commented Jul 3, 2017

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented Jul 4, 2017

terryjreedy commented Jul 4, 2017

terryjreedy commented Jul 2, 2017 •

edited

Loading

terryjreedy commented Jul 2, 2017 •

edited

Loading

terryjreedy commented Jul 3, 2017 •

edited

Loading

terryjreedy Jul 3, 2017 •

edited

Loading