Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-39411: pyclbr rewrite on AST #18103
Conversation
Great job! I left an initial batch of comments |
return | ||
|
||
name = node.targets[0].id | ||
child = copy.deepcopy(self.tree[node.value.id]) |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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).
Misc/NEWS.d/next/Library/2020-01-21-16-38-25.bpo-39411.9uHFqT.rst
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
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 |
This comment has been minimized.
This comment has been minimized.
Thanks for the reviews @pablogsal. (I couldn't commit suggestions through github UI because of some indentaton problem) |
isidentical commentedJan 21, 2020
•
edited by bedevere-bot
https://bugs.python.org/issue39411