PyCQA / redbaron

Bottom-up approach to refactoring in python
http://redbaron.pycqa.org/
694 stars 74 forks source link

Regression with if or def statements and trailing comments: #127

Closed kayhayen closed 7 years ago

kayhayen commented 7 years ago

Hello,

I am having a severe red baron regression parsing these kinds of codes:

    if python_version >= 340 and False: # TODO: Temporarily reverted:
        tmp_class = class_creation_function.allocateTempVariable(
            temp_scope = None,
            name       = "__class__"
        )
def displayTree(tree): # pragma: no cover
    # Import only locally so the Qt4 dependency doesn't normally come into play
    # when it's not strictly needed.
    from .gui import TreeDisplay

Seems the comments trailing the ":" are now considered illegal. But I swear to you, they are.

The version I am using from PyPI, is 0.6.3, I think 0.6.2 didn't do this at all, but I might be wrong.

Yours, Kay

Psycojoker commented 7 years ago

Hello,

This is in fact a bug in the latest release of Baron from what I see. It might come from this commit (not sure but I don't see a lot of other suspects) https://github.com/PyCQA/baron/commit/f2d3ecbf9c8d801a1f1b67a5909a9f95c917c266

Thanks for reporting :+1:

cc @ibizaman

kayhayen commented 7 years ago

Indeed, looks suspicious that commit, but can't you just git bisect baron to know precisely.

kayhayen commented 7 years ago

Oh, and really, please add a test where you successfully parse the CPython test suite and write this back, this would avoid regressions. Roughly 15% of the CPython test suite is affected by this bug, and that's such an easy (but slow) test. It will find lots of other things too.

Psycojoker commented 7 years ago

I'm not aware of this test suit, can you point it us to it? That would indeed greatly improve quality.

I don't have time right now for a bisect but that was indeed one of the available plans.

kayhayen commented 7 years ago

@Psycojoker You could use this and other branches:

https://github.com/python/cpython/tree/2.7/Lib/test

Obviously the (few) bad syntax examples you might want to ignore those. If I were you, I would just find ".py" files in the CPython source, try and compile with "-m compile" and if that works, try and parse with Redbaron, and reproduce a file from its tree, and check if it's identical. That ought to make sure that I have really low chances of coming up with syntax you didn't see yet.

Yours, Kay

Psycojoker commented 7 years ago

I used to do that on the top 100 projects of pypi but I've never automatized this test (as it was pretty long)

Thanks for the link :)

Psycojoker commented 7 years ago

Fixed by https://github.com/PyCQA/baron/commit/98acd9628702581f82db3f73bc3123f7f73bf75f

Psycojoker commented 7 years ago

I'm opening another ticket for the test suit. Thanks for reporting and your help :)

Psycojoker commented 7 years ago

Oh and I've made a release with this fix.

kayhayen commented 7 years ago

I am not so sure, this was a fix. Now I am getting this diff:

-def displayTree(tree): # pragma: no cover
+def displayTree(tree)# pragma: no cover:

Which of course isn't even valid Python anymore. The comment is moved to before the colon, which is wrong.

I am using this:

pip freeze | grep baron baron==0.6.4 redbaron==0.6.3

This should be the latest PyPI releases.

Yours, Kay

kayhayen commented 7 years ago

@Psycojoker Can you please re-open the issue, this has actually become worse. From crashing to corruption of the source.

Psycojoker commented 7 years ago

@kayhayen made another release that should fix this. I've also added test to ensure that dumps(parse(code)) == code to avoid this bug from happening again.

kayhayen commented 7 years ago

@Psycojoker Indeed the issue is now gone with 0.6.5, thanks a lot.