Skip to content

Conversation

@devdanzin
Copy link

I've made some changes to ast-comments while trying to get a version of ast.get_source_segment that roundtrips comments to use in radon.

They allow getting trailing comments from within function bodies. Unfortunately, it'll also incorporate comments that appear right after a function's body. I'm not sure this is of interest to include in ast-comments, but wanted to keep a record of the exploration and allow assessing whether (an improved version of?) this would be desirable.

@t3rn0 t3rn0 added the enhancement New feature or request label Sep 3, 2023
@t3rn0 t3rn0 self-requested a review September 3, 2023 18:53
@t3rn0
Copy link
Owner

t3rn0 commented Sep 7, 2023

Hi. Thanks for creating this PR.
I understand that in some cases proposed behavior may be convenient. But I think the current one is ok. Consider following example:

source = """
a = 1; b = 2
"""
tree = ast.parse(source)
for node in tree.body:  # len(tree.body) == 2
    print(ast.get_source_segment(node))

## prints ##
a = 1
b = 2

There are two separate nodes and there are two separate source segments. Now, let's change the b-node to a comment:

source = """
a = 1  # some comment
"""
tree = ast_comments.parse(source)
for node in tree.body:  # len(tree.body) == 2
    print(ast.get_source_segment(node))

## prints ##
a = 1
# some comment

Again, we have two separate nodes (Expr and Comment) and two separate source segments. I don't see any difference between the first and second examples, except for the ast node type.
If a Comment node is part of the body of some other node (function, forloop, etc. ), its source segment must also be part of the corresponding node's source segment:

source = """
def foo():  # some comment
    a = 1
"""
tree = ast_comments.parse(source)
for node in tree.body:  # len(tree.body) == 1 (!!!)
    print(ast.get_source_segment(node))

## prints ##
def foo():  # some comment
    a = 1

About PR:
From "testing" perspective I find that ast_comments.get_source_segment doesn't actually do anything. If you remove it pytest results won't change.

I recommend creating this modification on your side (in radon).

It's really great you raised this issue. Review helped me to find one minor bug 🙂

@t3rn0 t3rn0 closed this Sep 7, 2023
@t3rn0 t3rn0 removed their request for review September 7, 2023 17:38
@devdanzin
Copy link
Author

Thank you for reviewing this! I agree with your analysis, glad to hear that it helped spot a minor bug :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants