Skip to content

/ cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-39411: pyclbr rewrite on AST #18103

Open
wants to merge 6 commits into
base: master
from
Open

bpo-39411: pyclbr rewrite on AST #18103

wants to merge 6 commits into from
+144 −203

Conversation

@isidentical
Copy link
Contributor

isidentical commented Jan 21, 2020 •
edited by bedevere-bot

https://bugs.python.org/issue39411
@isidentical
bpo-39411: pyclbr rewrite on AST
Loading status checks…
2b755f4
@pablogsal pablogsal self-assigned this Jan 21, 2020
@pablogsal pablogsal self-requested a review Jan 21, 2020
@isidentical
add some helper functions to idlelib for removed private API
Loading status checks…
52fffd6
@isidentical isidentical requested a review from terryjreedy as a code owner Jan 21, 2020
@pablogsal
pablogsal requested changes Jan 21, 2020
View changes
Copy link
Member

pablogsal left a comment

Great job! 👌

I left an initial batch of comments
Doc/library/pyclbr.rst Outdated Show resolved Hide resolved
Lib/pyclbr.py Outdated Show resolved Hide resolved
Lib/pyclbr.py Outdated Show resolved Hide resolved
Lib/pyclbr.py Outdated Show resolved Hide resolved
Lib/pyclbr.py Outdated Show resolved Hide resolved
Lib/pyclbr.py Outdated
return

name = node.targets[0].id
child = copy.deepcopy(self.tree[node.value.id])

This comment has been minimized.

Sign in to view
Copy link
@pablogsal

pablogsal Jan 21, 2020

Member

Hummm.....this can be very inefficient as we would be copying the while tree for this id, no?

This comment has been minimized.

Sign in to view
Copy link
@isidentical

isidentical Jan 21, 2020

Author Contributor

we would only copy a Function object, which has really small memory footprint. If Function would have many children it would be inefficient but I think it is uncommon to put more than like 5-6 functions inside of another function.

This comment has been minimized.

Sign in to view
Copy link
@pablogsal

pablogsal Jan 21, 2020 •
edited

Member

I agree, but although minor I wish we could avoid the copy because we would do this for every function.

This comment has been minimized.

Sign in to view
Copy link
@isidentical

isidentical Jan 21, 2020 •
edited

Author Contributor

Ah, I think I can avoid this by manually reconstructing nodes, not sure if it would be the most efficient solution though.

This comment has been minimized.

Sign in to view
Copy link
@isidentical

isidentical Jan 21, 2020

Author Contributor

But we always need to deepcopy children I guess :/

This comment has been minimized.

Sign in to view
Copy link
@isidentical

isidentical Jan 21, 2020

Author Contributor 
        name = node.targets[0].id
        value = self.tree[node.value.id]
        parent = self.stack[-1]
        child = Function(
            value.module, name, value.file, node.lineno, parent, value.is_async
        )
        child.children = copy.deepcopy(value.children)
        parent._addchild(name, child)
        parent._addmethod(name, node.lineno)
``` what do you think about this?
Lib/pyclbr.py
return

for module in node.names:
try:

This comment has been minimized.

Sign in to view
Copy link
@pablogsal

pablogsal Jan 21, 2020

Member

I find this code a bit messy, maybe we should wrap or modify _readmodule to return a specific exception or None if something goes wrong to avoid this double nesting with retry.

This comment has been minimized.

Sign in to view
Copy link
@isidentical

isidentical Jan 21, 2020

Author Contributor

_readmodule is used directly by public readmodule and readmodule_ex interfaces. I dont think such a change is good because we might be simplify here but we need to add a way to raise that exceptions again in public APIs.

This comment has been minimized.

Sign in to view
Copy link
@pablogsal

pablogsal Jan 21, 2020 •
edited

Member

_readmodule is used directly by public readmodule and readmodule_ex interfaces. I dont think such a change is good because we might be simplify here but we need to add a way to raise that exceptions again in public APIs.

Let me think about what would be the best here, I still find this double try that is catching the same exception a code smell.

This comment has been minimized.

Sign in to view
Copy link
@terryjreedy

terryjreedy Jan 22, 2020

Member

The double try is messy but is part of the original. I worry more about having the same result as before.
Lib/test/test_pyclbr.py Show resolved Hide resolved
Misc/NEWS.d/next/Library/2020-01-21-16-38-25.bpo-39411.9uHFqT.rst Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 21, 2020

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 added 2 commits Jan 21, 2020
@isidentical
apply Pablo's suggestions
Loading status checks…
f3ff250
@isidentical
get default
Loading status checks…
4547e35
@isidentical

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

isidentical commented Jan 21, 2020

Thanks for the reviews @pablogsal. (I couldn't commit suggestions through github UI because of some indentaton problem)
@isidentical
avoid copying whole Function
Loading status checks…
bf12f16
@terryjreedy
terryjreedy requested changes Jan 22, 2020
View changes
Copy link
Member

terryjreedy left a comment

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.
Doc/library/pyclbr.rst Outdated Show resolved Hide resolved
Lib/idlelib/idle_test/test_browser.py Outdated Show resolved Hide resolved
Lib/test/test_pyclbr.py Show resolved Hide resolved
Lib/pyclbr.py Outdated Show resolved Hide resolved
Lib/pyclbr.py
return

for module in node.names:
try:

This comment has been minimized.

Sign in to view
Copy link
@terryjreedy

terryjreedy Jan 22, 2020

Member

The double try is messy but is part of the original. I worry more about having the same result as before.
@isidentical
put _nest_function, _nest_class inside pyclbr again, for tests
Loading status checks…
1359246
@isidentical isidentical force-pushed the isidentical:bpo-39411 branch from 310b00a to 1359246 Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal

@terryjreedy terryjreedy

Assignees

@pablogsal pablogsal

Labels
CLA signed awaiting changes
Projects
None yet
Milestone
No milestone
5 participants
@isidentical @bedevere-bot @pablogsal @terryjreedy @the-knights-who-say-ni
You can’t perform that action at this time.