bpo-1612262: IDLE: Class Browser shows nested functions and classes #2573

csabella · 2017-07-04T23:41:10Z

Original patch by Guilherme Polo.

https://bugs.python.org/issue1612262

mention-bot · 2017-07-04T23:41:12Z

@csabella, thanks for your PR! By analyzing the history of the files in this pull request, we identified @terryjreedy, @kbkaiser and @rhettinger to be potential reviewers.

csabella · 2017-07-04T23:43:09Z

The first commit is almost exactly the original patch, with the differences being that objects are now children and dict is tree. Terry, I thought you might like to see it before I added anything else, such as docstrings.

brettcannon · 2017-08-14T20:50:51Z

A fix for the busted news entry check is coming shortly.

brettcannon · 2017-08-14T20:55:42Z

I should also mention that if you want to merge this now then you can either add the "skip news" label or just simply ignore the failure (it isn't a required one and so you can still merge even with it failing). Otherwise if you wait 10 minutes then (hopefully) the PR I just submitted will be live and fix this.

brettcannon · 2017-08-14T21:12:49Z

OK, the news status issue is all fixed. Sorry about that. 😞

terryjreedy

I have not looked at tests yet. This works with configdialog, which has a class with a function with a function.

I experienced 1 glitch that I cannot reproduce. When I clicked on is_int, the function name in the browser changed to a methold of one of the classes. When I clicked elsewhere, it changed back.

terryjreedy · 2017-09-12T00:54:58Z

Lib/idlelib/browser.py

+    Node is the current node being traversed.  The return value is
+    a tuple with the first value being a dictionary of the
+    Class/Function instances of the node and the second is
+    a list of tuples with (lineno, name).


The docstring says nothing about the name parameter.

I've changed the docstring. I think he left name as a parameter because it was used it in existing code. Also, the values returned from this function are pretty confusing. I think he tried to pull it "as-is" out of the existing code and modify it as little as possible, but it's not very clear.

terryjreedy · 2017-09-12T00:56:42Z

Lib/idlelib/browser.py

+    """
+    items = []
+    children = {}
+    for key, cl in node.items():


I believe 'cl' can be either a class or function object. 'c_f' or 'obj' would be a clearer name.

Yes, this and the s were in the original code (as was the name key). I figured once the tests looked good, then another PR would be used to re-write the traversal code.

terryjreedy · 2017-09-12T01:04:27Z

Lib/idlelib/browser.py

+    children = {}
+    for key, cl in node.items():
+        if name is None or cl.module == name:
+            s = key


Abbreviating 'key' as 's' seems pointless and confusing. (I know, you copied it.) Unless you see something I don't, delete and use 'key' below.

I wasn't sure how much you wanted to refactor before knowing the tests were good.

terryjreedy · 2017-09-12T01:08:49Z

Lib/idlelib/browser.py

+            children[s] = cl
+    return children, items
+
+
 class ClassBrowser:


This should become ModuleBrowser here, 3 other places in file, and in pathbrowser. 'Class Browser' should also be change in mainmenu. We should do this in a separate issue and patch. Lets see if we can do this quickly.

Did you want the change to ModuleBrowser to happen before this current PR?

terryjreedy · 2017-09-22T07:37:37Z

Lib/idlelib/idle_test/test_browser.py

@@ -0,0 +1,236 @@
+""" Test idlelib.browser.
+"""


I should add coverage. 100% requires a mock that raises specific exception. I know mock_idle.Func can and presume the unittest mock can also.

I also want to revise blurb slightly.

terryjreedy · 2017-09-22T15:40:39Z

Cheryl, please take a look. Pending a recheck, I think this is ready. Effective coverage is over 90% (thanks) and remainder is not related to this issue. There are about 4 little patches to follow.

miss-islington · 2017-09-22T20:10:53Z

Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

csabella · 2017-09-22T20:42:19Z

Sorry, was just looking when you merged. Your changes made all the complex parts really simple. I'm glad my test didn't need too much of a rewrite. Maybe I'm getting the hang of it. :-)

One test I think I missed was making sure it sorted by line numbers.

…hon#2573) Original patches for code and tests by Guilherme Polo and Cheryl Sabella, respectively. (cherry picked from commit 058de11)

terryjreedy · 2017-09-22T21:03:59Z

Please finish looking at it, in case you see something I missed. My many commits were designed to show how I got from there to here. The hardest part was converting MethodBTI, renamed to ChildBTI but still based on the design constrained by Class.methods, to properly use the child Function and Class objects (the tree nodes) and their .children attribute.

You are getting pretty decent at the tests and your drafts are very helpful.

…es (GH-2573) (#3702) Original patches for code and tests by Guilherme Polo and Cheryl Sabella, respectively. (cherry picked from commit 058de11)

the-knights-who-say-ni added the CLA signed label Jul 4, 2017

csabella force-pushed the bpo1612262 branch 2 times, most recently from 50410ae to 61be9ae Compare July 7, 2017 23:03

csabella force-pushed the bpo1612262 branch 2 times, most recently from 63193a4 to 050975a Compare July 12, 2017 22:51

csabella force-pushed the bpo1612262 branch from 050975a to 46cef46 Compare July 21, 2017 12:45

csabella force-pushed the bpo1612262 branch from 46cef46 to 6dd3fa0 Compare August 4, 2017 19:34

terryjreedy mentioned this pull request Aug 14, 2017

Bedevere does not see new news file python/bedevere#42

Closed

brettcannon added skip news and removed skip news labels Aug 14, 2017

csabella force-pushed the bpo1612262 branch from dd33dd1 to a15bc99 Compare August 25, 2017 20:52

Mariatta requested a review from terryjreedy September 11, 2017 22:41

terryjreedy requested changes Sep 12, 2017

View reviewed changes

bedevere-bot added the awaiting changes label Sep 12, 2017

csabella and others added 5 commits September 12, 2017 19:23

bpo-1612262: IDLE: Class Browser for nested functions/classes

a7b6411

Initial test for TreeItem structure

1f4c547

More complete unit tests.

6763dfc

News blurb.

7d8b4a6

Improve docstring and variable names.

2992dfb

csabella force-pushed the bpo1612262 branch from af601aa to 2992dfb Compare September 12, 2017 23:24

bedevere-bot removed the awaiting changes label Sep 12, 2017

Make unittest pass without disabling htest.

694e942

terryjreedy approved these changes Sep 22, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 22, 2017

terryjreedy requested changes Sep 22, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 22, 2017

Whitespace

778f77d

python deleted a comment from bedevere-bot Sep 22, 2017

terryjreedy added 3 commits September 22, 2017 10:54

whitespace

32f573f

Add coverage.

a1d4574

Tweak blurb.

e40bf5d

terryjreedy approved these changes Sep 22, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 22, 2017

Make transform_children robust against repeated calls with same dict.

096944c

terryjreedy changed the title ~~bpo-1612262: IDLE: Class Browser for nested functions/classes~~ bpo-1612262: IDLE: Class Browser shows nested functions and classes Sep 22, 2017

terryjreedy merged commit 058de11 into python:master Sep 22, 2017

terryjreedy added needs backport to 3.6 and removed awaiting merge labels Sep 22, 2017

terryjreedy removed the needs backport to 3.6 label Sep 22, 2017

terryjreedy mentioned this pull request Sep 22, 2017

Hanging .... python/miss-islington#23

Closed

csabella deleted the bpo1612262 branch September 22, 2017 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-1612262: IDLE: Class Browser shows nested functions and classes #2573

bpo-1612262: IDLE: Class Browser shows nested functions and classes #2573

csabella commented Jul 4, 2017 •

edited by bedevere-bot

Loading

mention-bot commented Jul 4, 2017

csabella commented Jul 4, 2017

brettcannon commented Aug 14, 2017

brettcannon commented Aug 14, 2017

brettcannon commented Aug 14, 2017

terryjreedy left a comment

terryjreedy Sep 12, 2017

csabella Sep 12, 2017

terryjreedy Sep 12, 2017

csabella Sep 12, 2017

terryjreedy Sep 12, 2017

csabella Sep 12, 2017

terryjreedy Sep 12, 2017

csabella Sep 12, 2017

terryjreedy Sep 22, 2017 •

edited

Loading

terryjreedy commented Sep 22, 2017

miss-islington commented Sep 22, 2017

csabella commented Sep 22, 2017 •

edited

Loading

terryjreedy commented Sep 22, 2017

bpo-1612262: IDLE: Class Browser shows nested functions and classes #2573

bpo-1612262: IDLE: Class Browser shows nested functions and classes #2573

Conversation

csabella commented Jul 4, 2017 • edited by bedevere-bot Loading

mention-bot commented Jul 4, 2017

csabella commented Jul 4, 2017

brettcannon commented Aug 14, 2017

brettcannon commented Aug 14, 2017

brettcannon commented Aug 14, 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

terryjreedy Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

terryjreedy commented Sep 22, 2017

miss-islington commented Sep 22, 2017

csabella commented Sep 22, 2017 • edited Loading

terryjreedy commented Sep 22, 2017

csabella commented Jul 4, 2017 •

edited by bedevere-bot

Loading

terryjreedy Sep 22, 2017 •

edited

Loading

csabella commented Sep 22, 2017 •

edited

Loading