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

New algorithm inspired by original code with inline comments in fields #29

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

kapulkin
Copy link
Contributor

@kapulkin kapulkin commented Mar 23, 2024

Issue #13 fix

@t3rn0
Copy link
Owner

t3rn0 commented Mar 24, 2024

Wow. That's huge:)
Could you also write some tests showing what has changed? It will just be much easier to understand

ast_comments.py Show resolved Hide resolved
@kapulkin
Copy link
Contributor Author

kapulkin commented Mar 25, 2024

I tested new and old algorithms on examples below:

Example 1

code = """# start comment
if (False # inside comment
    and True): # condition comment
    # if comment
    a = 5
# before else comment
else: # false comment
    # else comment
    a = 6 # inline comment
# end comment"""

Output 1

Module(
    body=[
        Comment(
            value='# start comment',
            inline=False,
            lineno=1,
            col_offset=0,
            end_lineno=1,
            end_col_offset=15),
        If(
            test=BoolOp(
                op=And(),
                values=[
                    Constant(
                        value=False,
                        comment=Comment(
                            value='# inside comment',
                            inline=True,
                            lineno=2,
                            col_offset=10,
                            end_lineno=2,
                            end_col_offset=26),
                        lineno=2,
                        col_offset=4,
                        end_lineno=2,
                        end_col_offset=9),
                    Constant(
                        value=True,
                        comment=Comment(
                            value='# condition comment',
                            inline=True,
                            lineno=3,
                            col_offset=15,
                            end_lineno=3,
                            end_col_offset=34),
                        lineno=3,
                        col_offset=8,
                        end_lineno=3,
                        end_col_offset=12)],
                lineno=2,
                col_offset=4,
                end_lineno=3,
                end_col_offset=12),
            body=[
                Comment(
                    value='# if comment',
                    inline=False,
                    lineno=4,
                    col_offset=4,
                    end_lineno=4,
                    end_col_offset=16),
                Assign(
                    targets=[
                        Name(
                            id='a',
                            ctx=Store(),
                            lineno=5,
                            col_offset=4,
                            end_lineno=5,
                            end_col_offset=5)],
                    value=Constant(
                        value=5,
                        lineno=5,
                        col_offset=8,
                        end_lineno=5,
                        end_col_offset=9),
                    lineno=5,
                    col_offset=4,
                    end_lineno=5,
                    end_col_offset=9),
                Comment(
                    value='# before else comment',
                    inline=False,
                    lineno=6,
                    col_offset=0,
                    end_lineno=6,
                    end_col_offset=21)],
            orelse=[
                Comment(
                    value='# false comment',
                    inline=False,
                    lineno=7,
                    col_offset=6,
                    end_lineno=7,
                    end_col_offset=21),
                Comment(
                    value='# else comment',
                    inline=False,
                    lineno=8,
                    col_offset=4,
                    end_lineno=8,
                    end_col_offset=18),
                Assign(
                    targets=[
                        Name(
                            id='a',
                            ctx=Store(),
                            lineno=9,
                            col_offset=4,
                            end_lineno=9,
                            end_col_offset=5)],
                    value=Constant(
                        value=6,
                        lineno=9,
                        col_offset=8,
                        end_lineno=9,
                        end_col_offset=9),
                    comment=Comment(
                        value='# inline comment',
                        inline=True,
                        lineno=9,
                        col_offset=10,
                        end_lineno=9,
                        end_col_offset=26),
                    lineno=9,
                    col_offset=4,
                    end_lineno=9,
                    end_col_offset=9)],
            lineno=2,
            col_offset=0,
            end_lineno=9,
            end_col_offset=9),
        Comment(
            value='# end comment',
            inline=False,
            lineno=10,
            col_offset=0,
            end_lineno=10,
            end_col_offset=13)],
    type_ignores=[])

Example 2

code = """source = {
    # c1
    a: 1,
    b: 2,  # c2
}"""

Output 2

Module(
    body=[
        Assign(
            targets=[
                Name(
                    id='source',
                    ctx=Store(),
                    lineno=1,
                    col_offset=0,
                    end_lineno=1,
                    end_col_offset=6)],
            value=Dict(
                keys=[
                    Comment(
                        value='# c1',
                        inline=False,
                        lineno=2,
                        col_offset=4,
                        end_lineno=2,
                        end_col_offset=8),
                    Name(
                        id='a',
                        ctx=Load(),
                        lineno=3,
                        col_offset=4,
                        end_lineno=3,
                        end_col_offset=5),
                    Name(
                        comment=Comment(
                            value='# c2',
                            inline=True,
                            lineno=4,
                            col_offset=11,
                            end_lineno=4,
                            end_col_offset=15),
                        id='b',
                        ctx=Load(),
                        lineno=4,
                        col_offset=4,
                        end_lineno=4,
                        end_col_offset=5)],
                values=[
                    Constant(
                        value=1,
                        lineno=3,
                        col_offset=7,
                        end_lineno=3,
                        end_col_offset=8),
                    Constant(
                        value=2,
                        lineno=4,
                        col_offset=7,
                        end_lineno=4,
                        end_col_offset=8)],
                lineno=1,
                col_offset=9,
                end_lineno=5,
                end_col_offset=1),
            lineno=1,
            col_offset=0,
            end_lineno=5,
            end_col_offset=1)],
    type_ignores=[])

@margual56
Copy link

@t3rn0 Hello, I think kapulkin has made the requested changes. Maybe you could re-review this PR? Thank you!

@kapulkin
Copy link
Contributor Author

kapulkin commented Sep 6, 2024

Yes, I made an update with parsing bug fix.
So now the code is quite stable. I use it on a daily basis.

@margual56, thank you for pointing about the update.

@tristanlatr
Copy link

I believe it's still missing actual executable test cases in test_parse.py and test_unparse.py.

Also, since this proposal breaks the API of the library I think it should at least comes with the appropriate changes to the README.md file explaining the changes.

@t3rn0
Copy link
Owner

t3rn0 commented Dec 12, 2024

Sorry for such a late response.
This change seems broken to me. Example: there's no _Unparser definition which is used in unparse function (L241), and unparsing comments and putting them to their initial place is essential for #13.
+ there are no new test cases that would "freeze" the new features/fixes. Without it I see no reason to change anything.
I've made several attempts to work with this PR and the issue, but so far without success.

@kapulkin
Copy link
Contributor Author

kapulkin commented Dec 17, 2024

@t3rn0 thank you for the review. You are right about _Unparser, this is part of outdated code, I will remove it.

Indeed the code had also a few issues. During intensive code usage I found them and fixed few month ago on my machine. I updated the pull request now. Sorry for late update.

I agree, that future work on tests is needed to complete pull-request. Any hep here is appreciated. I will try to add tests from my side too.

@kapulkin kapulkin requested a review from t3rn0 December 17, 2024 21:34
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.

4 participants