Skip to content
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 4 commits into
base: master
from
Open

Conversation

@isidentical
Copy link
Contributor

isidentical commented Jan 21, 2020

@pablogsal pablogsal self-assigned this Jan 21, 2020
@pablogsal pablogsal self-requested a review Jan 21, 2020
@isidentical isidentical requested a review from terryjreedy as a code owner Jan 21, 2020
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
return

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

This comment has been minimized.

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.

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.

return

for module in node.names:
try:

This comment has been minimized.

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.

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.

self.checkModule('test.pyclbr_input', ignore=['om'])

def test_nested(self):
mb = pyclbr
# Set arguments for descriptor creation and _creat_tree call.
m, p, f, t, i = 'test', '', 'test.py', {}, None
source = dedent("""\
def f0:

This comment has been minimized.

Copy link
@pablogsal

pablogsal Jan 21, 2020

Member

Was this code running before? This will raise SyntaxError because is invalid, no? It is running now?

This comment has been minimized.

Copy link
@isidentical

isidentical Jan 21, 2020

Author Contributor

This is something I couldn't figure out. This test was working because of tokenization, and started failing under AST. So I fixed the invalid syntax but I'm not sure if this is a breaking change for this module, I know people may depend on this but this is a low used module and offical documentation says this should be only work on python source codes (which def f0: isn't a python code).

@bedevere-bot

This comment has been minimized.

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

This comment has been minimized.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.