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

Refactor patchedast #594

Merged
merged 10 commits into from Dec 21, 2022
Merged

Refactor patchedast #594

merged 10 commits into from Dec 21, 2022

Conversation

Alex-CodeLab
Copy link

Description

Some refactoring.
It should not change any behavior. (perhaps a tiny bit faster )

@lieryan
Copy link
Member

lieryan commented Dec 14, 2022

Hi @Alex-CodeLab, thank you for taking the time to create this PR. However, I don't think many of these refactoring are desirable.

  1. For a pair of brackets/quotes, then it is better for readability that the way the starting and ending token are pushed to the children should match for symmetry.

    # good, both starting and ending token uses append
    children = []
    children.append("{")
    ...
    children.append("}")
    
    # good, both starting and ending token are handled by the comprehension
    children = [
        "{", 
        *do_something_with(node.generators),
        "}"
    ]
    
    # bad, yes, it may save a couple lines, but it lacks symmetry
    children = ["{"]
    ...
    children.append("}")
  2. In general, children.append() should be used to push individual unrelated tokens, children.extend() should only be used when adding childrens that are one-to-many or for very closely related token:

    # good, as `node.generators` is a list
    children.extend(node.generators)
    
    # fine, "async def" functionally acts like one token, even though it is composed of two words
    children.extend(["async", "def"])
    
    # bad, this is pushing functionally unrelated token, which breaks hierarchy
    def _excepthandler(self, node):
        ...
        children.extend((self.semicolon_or_as_in_except, node.name))
  3. Don't use side-effects with inline-if:

    # bad, inline-if shouldn't have side effect
    children.extend(["async", "def"]) if is_async else children.append("def")
    
    # good, no side effect
    if is_async:
        children.append("def")
    else:
        children.extend(["async", "def"])
    
    # better, if you want to do it with one-liner
    children.extend(["async", "def"] if is_async else ["def"])

If you revise the PR with these in mind, then we can go ahead with merging this PR.

@Alex-CodeLab
Copy link
Author

  1. true, it can be somewhat more readable when it is on multiple lines, however doing it this way is also slightly more performant since it avoids the function calls to append. To me it looks a bit weird (inefficient) to create empty lists and then add elements one by one, I guess it is a matter of taste. But I understand your point.

  2. Not sure I understand what you mean ; the result is the same, right?

  3. yes, makes sense.

@lieryan
Copy link
Member

lieryan commented Dec 14, 2022

The result is the same, but the readability isn't. There is a visual and conceptual hierarchy in the syntax, and the code is much harder to comprehend when the way the code is written does not reflect that hierarchy.

The performance difference between append() and extend() for code in patchedast.py is basically negligible (patchedast.py isn't a performance sensitive code), so readability should be the main factor on how you should structure the code here.

@lieryan lieryan merged commit d36d381 into python-rope:master Dec 21, 2022
@lieryan lieryan added this to the 1.7.0 milestone Dec 21, 2022
@Alex-CodeLab
Copy link
Author

thanks.
(sorry I didnt have time to fix it myself)

@lieryan
Copy link
Member

lieryan commented Dec 22, 2022

No worries, thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants