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

inconsistency for comment at start of file #44

Closed
ze42 opened this issue Jun 16, 2014 · 10 comments
Closed

inconsistency for comment at start of file #44

ze42 opened this issue Jun 16, 2014 · 10 comments
Labels

Comments

@ze42
Copy link

ze42 commented Jun 16, 2014

Hello,

Little unconsistency when parsing comment at start of file.

If it starts with a newline, we alternate endl/comment:

parse('\n#foo\n#bar\n')
Out[7]: 
[{'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'},
 {'formatting': [], 'type': 'comment', 'value': '#foo'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'},
 {'formatting': [], 'type': 'comment', 'value': '#bar'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'}]

If it starts with a comment, we get a endl that includes the comment as formatting.

parse('#foo\n#bar\n')
[{'formatting': [{'formatting': [], 'type': 'comment', 'value': '#foo'}],
  'indent': '',
  'type': 'endl',
  'value': '\n'},
 {'formatting': [], 'type': 'comment', 'value': '#bar'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'}]

Not too sure what the proper behaviour is supposed to be, but I guess it should be something similar in both cases.

@ibizaman ibizaman added the bug label Jun 16, 2014
@ibizaman
Copy link
Collaborator

I tested your example and have the same behaviour.

IMHO the second case should give what you thought:

[{'formatting': [], 'type': 'comment', 'value': '#foo'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'},
 {'formatting': [], 'type': 'comment', 'value': '#bar'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'}]

@Psycojoker
Copy link
Member

Thanks for reporting :)

Indeed, this should be fixed in this function https://github.com/Psycojoker/baron/blob/master/baron/grammator.py#L52-64

But to warn you, that's the result of an edge case of the Baron approach. Since the comments aren't present in the python grammar and that they can appears anywhere, their handling is quite complex because they need to be "unpacked" from the tokens that are present in the grammar node (in this case, they are always on an ENDL token).

@ibizaman
Copy link
Collaborator

I think there should be a fix here: https://github.com/Psycojoker/baron/blob/master/baron/formatting_grouper.py#L120-122.

At that moment, the COMMENT is placed inside an ENDL node although there are no endl node there. So the fix can't happen after that because then the information that no ENDL was there is lost.

In the second example given, the input sequence to the group_generator function is:

[
    ('COMMENT', '#foo'),
    ('ENDL', '\n'),
    ('COMMENT', '#bar'),
    ('ENDL', '\n'),
    ('ENDMARKER', ''),
    None
]

And its output is:

[
    ('ENDL', '\n', [('COMMENT', '#foo')], [('COMMENT', '#bar')]),
    ('ENDL', '\n'),
    ('ENDMARKER', '')
]

For comparison, here's the input and output with the first example (i.e. with a leading '\n'):

[
    ('ENDL', '\n'),
    ('COMMENT', '#foo'),
    ('ENDL', '\n'),
    ('COMMENT', '#bar'),
    ('ENDL', '\n'),
    ('ENDMARKER', ''),
    None
]
[
    ('ENDL', '\n', [], [('COMMENT', '#foo')]),
    ('ENDL', '\n', [], [('COMMENT', '#bar')]),
    ('ENDL', '\n'),
    ('ENDMARKER', '')
]

I tried to fix this but no luck for now...

@gtors
Copy link

gtors commented Aug 30, 2016

I replace group_generator (from formatting_grouper.py) with:

def group_generator(sequence):
    iterator = FlexibleIterator(sequence)
    current = None, None
    while not iterator.end():
        current = next(iterator)

        if current is None:
            return

        # -------------> Remove COMMENT from here <-------------
        if current[0] in ("SPACE") and iterator.show_next() and iterator.show_next()[0] in GROUP_SPACE_BEFORE:
            new_current = next(iterator)
            current = (new_current[0], new_current[1], [current])

        if current[0] in GROUP_SPACE_AFTER + STRING and\
            # -------------------------------------------------> And here <-------
            (iterator.show_next() and iterator.show_next()[0] in ("SPACE")) and\
    #... rest part of function

After that, parse('#foo\n#bar\n') works as expected:

[{'formatting': [], 'type': 'comment', 'value': '#foo'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'},
 {'formatting': [], 'type': 'comment', 'value': '#bar'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'}]

And dumps also works well:

dumps([{'formatting': [], 'type': 'comment', 'value': '#foo'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'},
 {'formatting': [], 'type': 'comment', 'value': '#bar'},
 {'formatting': [], 'indent': '', 'type': 'endl', 'value': '\n'}])

So, why COMMENT need be grouped at all?

@ibizaman
Copy link
Collaborator

@gtors nice catch! Thanks for figuring this out!

@bootandy
Copy link
Contributor

bootandy commented Dec 4, 2016

I have just tried gtors ' change and it seems to be correct. Is there any reason this fix doesn't have a pull request? If not I will create one.

@ibizaman
Copy link
Collaborator

ibizaman commented Dec 5, 2016

@bootandy no, just not having much time to devote to baron lately. Sorry about that. 😭

@Psycojoker
Copy link
Member

Psycojoker commented Dec 6, 2016

Me neither :/

I'll try to do a small bugfix release next weekend with this fix and PyCQA/redbaron#118 but can't promise..

RedBaron is a really concentration demanding project and I don't have the time I need to really get back into it right, sorry about that.

@rojaster
Copy link

rojaster commented Dec 8, 2016

I fixed it for my fork of baron :) and hey , another guy found similar bug

@ibizaman
Copy link
Collaborator

@bootandy Thanks for the PR @ze42 Closing as it's fix. Don't hesitate to reopen if you have any other issue.

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

No branches or pull requests

6 participants