bpo-39411: pyclbr rewrite on AST

[isidentical]

https://bugs.python.org/issue39411

[pablogsal]

Great job! 👌

I left an initial batch of comments

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[isidentical]

Thanks for the reviews @pablogsal. (I couldn't commit suggestions through github UI because of some indentaton problem)

[terryjreedy]

Timing pyclbr is a bit tricky because it caches its results, so one call per process. I use tkinter (.init) as longest module I know of, with lots of methods. New code is about twice as fast with current dev debug interpreter. I highly approve ;-)

f:\dev\3x>python -c "import time, pyclbr; start=time.time(); pyclbr.readmodule_ex('tkinter'); print(time.time()-start)"
Running Debug|Win32 interpreter...
1.8749518394470215

f:\dev\3x>git checkout pr_18103
Switched to branch 'pr_18103'

f:\dev\3x>python -c "import time, pyclbr; start=time.time(); pyclbr.readmodule_ex('tkinter'); print(time.time()-start)"
Running Debug|Win32 interpreter...
0.921851396560669

'Old' time with normal installed 64 bit 3.9.0a2 is .5+ so new time might be at least .2 seconds faster, which is subjectively significant.

I need to read the NodeVisitor doc to properly understand the subclass.

[isidentical]

@terryjreedy could you review again the current state of this PR?

[terryjreedy]

I would like to resolve nesting issues before I review _ModuleBrowser

Unless you ask me to revise directly, please git merge upstream/master

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[terryjreedy]

Summary change request: revert change to test.pyclbr_input and remove new visit_Assign and its helper method.

When I wrote the comments on visit_Assign and its helper function, I misinterpreted the purpose to be to add alias names for functions defined in classes, like the example given, which the addition does not do, and noted that there are other missing alias cases. These comments are somewhat obsolete for this issue, but relevant for a possible new issue, so I left them as is.

I subsequently removed visit_Assign and got this test failure.

l1=['m', 'sm', 'cm']
l2=['om', 'f', 'm', 'sm', 'cm']
ignore={'om', 'object'}
class=<class 'test.pyclbr_input.C'>
F.....
========================================================
FAIL: test_decorators (__main__.PyclbrTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Programs\Python39\lib\test\test_pyclbr.py", line 156, in test_decorators
    self.checkModule('test.pyclbr_input', ignore=['om'])
  File "C:\Programs\Python39\lib\test\test_pyclbr.py", line 121, in checkModule
    self.assertListEq(foundMethods, actualMethods, ignore)
  File "C:\Programs\Python39\lib\test\test_pyclbr.py", line 31, in assertListEq
    self.fail("%r missing" % missing.pop())
AssertionError: 'f' missing

With test_decorators commented out (or pyclbr_input properly restored), all other tests pass. This is the only use of pyclbr_input. The primary purpose, as the test name indicates, is to test that static and class methods get listed.

I now understand that the purpose of visit_Assign was add a Function for class aliases of module functions to fix the test bug you introduced by uncommenting the line that I think should have just been deleted. Perhaps presence of that line reflected an idea of someday adding aliases, but it seems at least as likely that the idea was to one day fix is_method so that not adding aliases could be tested. In any case, adding them should be a new, separate issue.

[isidentical]

I now understand that the purpose of visit_Assign was add a Function for class aliases of module functions to fix the test bug you introduced by uncommenting the line that I think should have just been deleted. Perhaps presence of that line reflected an idea of someday adding aliases, but it seems at least as likely that the idea was to one day fix is_method so that not adding aliases could be tested. In any case, adding them should be a new, separate issue.

I see. The reason that I uncommented this is a bit of showcase of an advantage using AST over the raw tokens (beside the better code quality, consistency, and a few little bugfixes). Will revert that part for the future.

[terryjreedy]

@pablogsal Batuhan has satified my change requests. How about yours?
@isidentical You can merge this yourself now when ready. I missed the inverted default you just fixed.
Add me as nosy to any followup issues.

[pablogsal]

LGTM

[bedevere-bot]

@isidentical: Please replace # with GH- in the commit message next time. Thanks!