IDLE : Display function argument list in ModuleBrowser

[22200024-de1a-4081-ad85-2ac04e6b54d2]

BPO	20827
Nosy	@terryjreedy, @roseman, @rovitotv, @csabella
Files	classbrowser-improvments.patch classbrowser-improvements-v2.patch patch.txt

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['expert-IDLE', 'type-feature', '3.7'] title = 'IDLE : Display function argument list in ClassBrowser' updated_at = user = 'https://bugs.python.org/SaimadhavHeblikar' ``` bugs.python.org fields: ```python activity = actor = 'taleinat' assignee = 'none' closed = False closed_date = None closer = None components = ['IDLE'] creation = creator = 'Saimadhav.Heblikar' dependencies = [] files = ['34268', '34492', '47166'] hgrepos = [] issue_num = 20827 keywords = ['patch'] message_count = 10.0 messages = ['212555', '213193', '213264', '213333', '213722', '213985', '302819', '302824', '302970', '303187'] nosy_count = 5.0 nosy_names = ['terry.reedy', 'markroseman', 'Todd.Rovito', 'Saimadhav.Heblikar', 'cheryl.sabella'] pr_nums = [] priority = 'normal' resolution = None stage = 'test needed' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue20827' versions = ['Python 3.6', 'Python 3.7'] ```

[22200024-de1a-4081-ad85-2ac04e6b54d2]

The proposed patch 1.implements a TODO for ClassBrowser . it was (sic)show function argument list? (have to do pattern matching on source).my patch does not do pattern matching in the real sense of the phrase.it uses "imp" to import the module source and "inspect.getargspec" to get the function argument list.a new function called FormatArgumentList() is used to beautify and display only the relevant arguments. in short,it displays the arguments for that function.previously, it was def foo(...) now it is, def foo(bar1,bar2,*spam,**spam2,defaults=(5,))

2.Adds a human test dialog for ClassBrowser.

Currently there are no specific tests for ClassBrowser. hence,i have also created test stubs for ClassBrowser.(ns:it is NOT present in this patch). One way or another, will send in the patch which will add tests.(i'm waiting to know whether we go forward on 1 and 2).

[terryjreedy]

ClassBrowser has multiple problems. I agree with the goal of given signatures. But the patch has these problems.

1. Idle is getting out of the business of formatting signatures. If we use inspect, '(...)' should simply be replaced (in 3.3+) by str(inspect.signature(ob)). (I plan to change calltips to do this instead of using two older inspect functions.) Using inspect creates the problems of getting ob.

2 It we go that route, the module should be imported just once, not once per tree item. Each class should be retrieved just once, not once per method.

3. The patch does not handle nested classes (or their methods). If nested classes are found in the dictionary of 'methods' of the enclosing class, this should be possible to keep track of.

I believe the intent of the comment in the ClassBrowser source was to use the 'def' line number provided by pyclbr to find the open and close parentheses of the header and copy the stuff in between. The open ( after the name is easy. Finding the closing ) is easy in the simple case, but hard in the general case because of nested parens in expressions and unmatched paren in strings and comments. Or because of ':' being used in dict pairs and in strings and comments, as well as terminating the header. I see why it was not done.

On the other hand, if the output of tokenize.tokenize is used, as in pyclbr, then matching parens is easy, as strings and comments are distinguished from ( and ) as operators. This suggests that extracting the signature string belongs in pyclbr.

I am tempted to pull the code that parses the token stream out of pyclbr and adapt it to our uses. We could easily make a Text widget 'readline' so tokenize could get lines directly from the text widget. (We really should do that anyway for iterating through in-memory text. Format: string trailing whitespace already does that.) That would make most of this suggested todo "- add popup menu with more options (e.g. doc strings, base classes, imports)" possible. Docstrings could be kept as a view objest with start and stop positions until requested.

-- The current main() has itself multiple problems. It does not work because PyShell.flist does not exist when it is called (the same attribute reference does work in the OnDoubleClick methods.

I presume that the backup line 'file = sys.argv[0]' is for running the file as main from the command line, as a test. That would not work on my Windows systems because x.py is not executable.

Since 'if sys.stdin is sys.__stdin__:' is not currently try, the mainloop does not run anyway.

Issue bpo-18104 is about human-run tests. I have not yet decided exactly what to do, but I do not want to call them 'main' in the module file itself, and probably want to move them elsewhwere.

[22200024-de1a-4081-ad85-2ac04e6b54d2]

>1. Idle is getting out of the business of formatting signatures. If we >>use inspect, '(...)' should simply be replaced (in 3.3+) by >>str(inspect.signature(ob)). (I plan to change calltips to do this >>instead of using two older inspect functions.) Using inspect creates >>the problems of getting ob.

This means instead of manually formatting,we use inspect.signature()?(Shall i try to first it on calltips and get back with the result?)

>3. The patch does not handle nested classes (or their methods). If >>nested classes are found in the dictionary of 'methods' of the >>enclosing class, this should be possible to keep track of.

I think this is because pyclbr is responsible for parsing the source.(This patch only tries to extract the signature,given the method/class).bpo-1612262 also seems to convey that pyclbr is unable to detect nested classes.

So i will try to make changes to pyclbr , to detect nested classes.

After i complete the above two,i will report here and try to work on

>- add popup menu with more options (e.g. doc strings, base classes, >>imports)" possible. Docstrings could be kept as a view objest with >>start and stop positions until requested.

Also, your suggestion to import the class/methods only once ,will improve the performance,especially on bigger files.I'll modify the patch to reflect this too.

[terryjreedy]

inspect.signature is now the official way to get signatures (sort of the 3rd). It now works with some builtins, and will work with most by the time 3.5 is released. It includes removing the first parameter of bound methods. If it does not work correctly for some case (as 'correctly' is generally agreed on), an issue should filed to fix it.

bpo-19903 is about changing calltips, but only for 3.4+. The tests will have to be changed and can perhaps be reduced. I have not done it yet because .signature was still being worked on until a few weeks ago.

From my reading of pyclbr, I expected it to detect nested classes. I saw specific code to ignore nested functions, as they are generally not accessible from outside the outer function, whereas nested classes are as accessible as methods. But a simple test with class C: class inner: def innerf(): pass def Cfunc(): pass shows that I was wrong. Class inner is not listed, but it should be. I think it would be simple to treat it as if it were a method, but use a Class instance. If you want to work on this, open a new issue.

There are three other problems I encountered while testing.

1. If I edit the file and open CB, it ignores an existing (obsolete) CB window and open a new one instead of reusing the existing window or closing it first. Sometimes it opens the new one exactly on top of the old one.

2. If I forget to save, it uses the obsolete data on disk. This is why I like the idea of using the live text. Barring that, I think there should be an message box offering to save and open the window or cancel.

Fixing either of these would involve the code that responds to Alt-C or the menu entry, which is somewhere other than ClassBrowser.py. Again, these would be new issues.

3. On my screen, each line cuts off the bottom of the line above. This might be due to the icons being 'too big' (but I think they are a standard 16x16). Or perhaps me using Windows' 125% text size setting. I have no idea whether this can be fixed in Idle's use of the Tree widget or is ultimately a bug in the Tk code.

These all bother me more than having to double click a function name to get its signature.

[22200024-de1a-4081-ad85-2ac04e6b54d2]

I have added a improved patch. What this patch does:

1. resolves issue 1 of msg213193 - uses inspect. signature() instead of inspect.getargspec.

2. resolves issue 2 of msg213193 - the module is imported only once per ClassBrowser instance.

3. resolves issue 2 of msg213333 - the classbrowser display is based on the current EditorWindow text and not based on whats on disk. All changes to reflect this have been made in ClassBrowser.py and pyclbr, therefore i did not create a new issue.

4. resolves issue 1 of msg213333 - though i feel there are better ways to do this than this patch - which manually close and create a new ClassBrowser instance

what this patch does NOT do:

1. the test_pyclbr fails after the proposed changes to pyclbr,(which is just adding a new keyword argument to _readmodule,readmodule_ex. i have circled it down to this. the only other change to pyclbr is an extra "if" statement.) i have tried to modify the tests to reflect the same, but to no avail. if you can give me some hints on how to move forward, i'll be quick to implement the tests. here is the test error ->

test_decorators (test.test_pyclbr.PyclbrTest) ... B FAIL test_easy (test.test_pyclbr.PyclbrTest) ... ok test_issue_14798 (test.test_pyclbr.PyclbrTest) ... ok test_others (test.test_pyclbr.PyclbrTest) ... BytesHeaderParser FAIL

\====================================================================== FAIL: test_decorators (test.test_pyclbr.PyclbrTest) ----------------------------------------------------------------------

Traceback (most recent call last):
  File "/media/development/cpython/Lib/test/test_pyclbr.py", line 152, in test_decorators
    self.checkModule('test.pyclbr_input', ignore=['om'])
  File "/media/development/cpython/Lib/test/test_pyclbr.py", line 139, in checkModule
    self.assertHaskey(dict, name, ignore)
  File "/media/development/cpython/Lib/test/test_pyclbr.py", line 43, in assertHaskey
    self.assertIn(key, obj)
AssertionError: 'B' not found in {}

====================================================================== FAIL: test_others (test.test_pyclbr.PyclbrTest) ----------------------------------------------------------------------

Traceback (most recent call last):
  File "/media/development/cpython/Lib/test/test_pyclbr.py", line 167, in test_others
    cm('email.parser')
  File "/media/development/cpython/Lib/test/test_pyclbr.py", line 139, in checkModule
    self.assertHaskey(dict, name, ignore)
  File "/media/development/cpython/Lib/test/test_pyclbr.py", line 43, in assertHaskey
    self.assertIn(key, obj)
AssertionError: 'BytesHeaderParser' not found in {}

---

Ran 4 tests in 15.437s

[22200024-de1a-4081-ad85-2ac04e6b54d2]

The old file had a typo and i have removed it.

The new file does everything stated in msg213722 and also has the errors removed. I have added tests for the little change in code to pyclbr.its called test_string_source. All the tests related to pyclbr pass

test_decorators (main.PyclbrTest) ... ok test_easy (main.PyclbrTest) ... ok test_issue_14798 (main.PyclbrTest) ... ok test_others (main.PyclbrTest) ... ok test_string_source (main.PyclbrTest) ... ok ---------------------------------------------------------------------- Ran 5 tests in 17.489s OK

The patch in its current form is fully functional.Do let me if any part requires modification.

[csabella]

Terry,

I had an idea about this when I was working on the docstrings and the original patches.

I've attached a diff for the first step of my suggested patch. I think you get the idea where I'm going with it, but I just call the same code that pyclbr uses to get the super class and reuse it functions. Then I added a new value to Function called self.args, which is similar to self.super for Class. I would still need to document it (obviously) and clean it up, but the new function _parse_args() is almost exactly the same as before, except that I've added a check for 'class' before seeing if it's already in the tree (line 298).

I've also modified _main(), so you can see the results from the command line. (python pyclbr.py)

If you like this approach, then I'll look at it more closely to make sure it's correct and I'll modify the tests.

[terryjreedy]

Miscellaneous comments:

1. ClassBrowser.main, changed in Heblikar's patch and referred to in the discussion, was the predecessor to the current browser._module_browser htest function.

I have not looked at the patch in detail, but it would have to be revised to be applied today.

2. pyclbr.Class and .Function look like public APIs (unlike _Object) and so I presume we must preserve back-compatibility by adding new args at the end of the arg list.

3. Changing pyclbr involves other core devs and possibly permissions and discussions. If we do patch it, we should also use guaranteed ordered dicts. 3.7.0b1 is expected early next January.

Pyclbr is outdated at least since 3.5 since it ignores 'async'. (At least that does not cause a following def to be skipped.) It should really have an AFunction subclass or a marker on Function so Module Browser could then prefix 'def' with 'async'.

4. Bases and abstract arguments are both sequences between ()s. But for pyclbr and browser, the analogy does not go much further. Pyclbr tries to replace string names of bases with Class objects. Browser uses this to prefix bases with paths prior to turning the sequence back to a string. If it were not for that, having the stuff between ( and ) parsed would be a nuisance. All browser currently needs is everything between ( and ) as a single string. Actually, taking annotations into account, everything between ( and :, minus extraneous whitespace.

5. Pyclbr intentionally reads code but does not run it. Must we keep that feature? Or should be embrace and extend it?

Except for the current file, IDLE (currently) can only do completions and calltips for code that has been imported (and run) into the current user process. Could it do as well or better without imports, perhaps by analyzing but not running other code in the IDLE process?

When pyclbr and IDLE were written, the only other no-run option was parsing to a barely readable concrete syntax tree. Since a decade ago, ast has been available. I believe that an ast tree contains all the information in a pyclbr tree. That means that it should be possible to dump all the hand-crafted parsing in pyclbr and re-implement it by extracting the same info from an ast tree, using the ast node visitors.

What I am thinking is that IDLE might use ast for module browser, completions, and calltips. If you are not familiar with ast, but want to learn, run something like

import ast
code = '''
import itertools as it
class a(it.count):
    def __init__(self, n): pass
    def extra(self, m:int) -> int: pass
'''
tree = ast.parse(code)
for node in ast.walk(tree):  
    print(node)  # This produces same as ast.dump(tree)
    print(node._fields)

and then skim the official doc and look at least this page of the resource recommended at the end. https://greentreesnakes.readthedocs.io/en/latest/nodes.html#function-and-class-definitions

[terryjreedy]

Having fixed the nested functions and classes issue, and thought some more, I have two other concerns with adding signatures in the browser, and a different solution.

1. Time: When requested, the browser window comes up immediately with the module node. There is a noticeable delay before the top level items are displayed, and it is longer than before. Since pyclbr was already patched to deliver the information on nested items, this puzzles me, and I want to investigate the cause before possible slowing display even longer.

2. Space: For browser tree items, base 'Tree' is expanded to 'idlelib.tree.Tree'. With one base, this will usually not an issue. With multiple bases, it could be, in the sense of making the single line too long. Signatures, like calls, can well be too long for a single line.

Alternatives:

1. Module Browser opens the file. Single clicking a line in the browser moves the selection highlight in the browser. Double clicking a line expands or contract the node if it is expandedable, and it highlights the corresponding line in the editor. So the new feature is not *necessary* for a user to get the information.

But we can make it easier. First, move the editor highlight when moving the browser hightlight. Second, move both highlights with the Up and Down keys (instead of just scrolling. Expand/contract with Right and Left keys. Third, with a bit more work, the entire header could be highlighted. Fourth, position browser box and editor side-by-side with tops aligned, instead of overlapping. (Note: the positioning would be automatic if IDLE had side-by-side tabbed panes.)

2. Signatures might be displayed in a pop-up box. But I like 1. above better.

So I am inclined to reject adding to the Python-code browser in favor of improving the coordination between browser and editor.

If we ever add the ability to browse imported modules by introspection, then making signatures, *and* docstrings, would be needed -- and easy. (This would be be aimed at, but not limited to, non-Python coded modules.)

[terryjreedy]

As with removing icons (bpo-25090), moving 'highlight line in editor' from double click to single click is not trivial. Single click behavior is baked into tree. TreeNode. TreeItems can only override or augment double-click behavior. I am deferring 'improve visibility of function argument list until I seriously look at ttk.Treeview (bpo-31552).