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

Fix unexpected whitespace removal outside line range #1102

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

limwz01
Copy link

@limwz01 limwz01 commented Jun 6, 2023

  • save the whitespace prefix in the pytree node even for those occurring with comments
  • disable the comment splicer's removal of trailing whitespace
  • add back the whitespace before emitting unformatted line
  • remove trailing whitespace in comments for enabled lines later when printing

TODO:

  • extensive testing is necessary as this method is like a hack
  • this still will remove trailing whitespace in line just before line range and will ignore trailing whitespace in last line of line range

* save the whitespace prefix in the pytree node even for those occurring with comments
* disable the comment splicer's removal of trailing whitespace
* add back the whitespace before emitting unformatted line
* remove trailing whitespace in comments for enabled lines later when printing

TODO: this still will remove trailing whitespace in line just before line range and will ignore trailing whitespace in last line of line range
@bwendling
Copy link
Member

Could you give an example of what this is fixing?

@limwz01
Copy link
Author

limwz01 commented Oct 25, 2023

The example is in the linked issue.

@Spitfire1900
Copy link
Contributor

Resolve conflicts

@limwz01
Copy link
Author

limwz01 commented Jan 22, 2024

After a long while of using this with only the problems mentioned in my first post, I've found a further problem that occurs with the following:

def main():
    if 1:
        if 1:
            pass

        # extra
        elif 1:
            pass
    pass

Applying the modified yapf on anywhere except including the extra comment line, we will get extra trailing space on the line before the extra comment line. I've not figured out the bug yet.

Copy link
Member

@bwendling bwendling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs testcases.

@@ -19,18 +19,19 @@
Reformat(): the main function exported by this module.
"""

import json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove.


from yapf_third_party._ylib2to3 import pytree
from yapf_third_party._ylib2to3.pgen2 import token
import sys
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove.


from yapf.pytree import pytree_utils
from yapf.yapflib import format_decision_state
from yapf.yapflib import format_token
from yapf.yapflib import line_joiner
from yapf.yapflib import style
from yapf_third_party._ylib2to3 import pytree
from yapf_third_party._ylib2to3.pgen2 import token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove.

@@ -72,6 +73,21 @@ def Reformat(llines, lines=None):
if lline.disable or _LineHasContinuationMarkers(lline):
_RetainHorizontalSpacing(lline)
_RetainRequiredVerticalSpacing(lline, prev_line, lines)
if not _LineHasContinuationMarkers(lline):
for token in lline.tokens:
# print(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the debugging comments.

comment_prefix,
comment_lineno,
comment_column,
"",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use single-quotes for consistency.

@@ -85,6 +85,7 @@ class PyTreeUnwrapper(pytree_visitor.PyTreeVisitor):
def __init__(self):
# A list of all logical lines finished visiting so far.
self._logical_lines = []
self.prefix = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single quotes.

if leaf.type in _WHITESPACE_TOKENS:
self._StartNewLine()
if leaf.type == grammar_token.NEWLINE:
self.prefix += "\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single quotes.

format_token.FormatToken(leaf, pytree_utils.NodeName(leaf)))
format_token.FormatToken(leaf, pytree_utils.NodeName(leaf),
self.prefix))
self.prefix = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

@@ -206,6 +223,7 @@ def _VisitNodeRec(node):
def _CreateCommentsFromPrefix(comment_prefix,
comment_lineno,
comment_column,
prefix,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a default prefix='' so you only have to pass it in when it's not empty.

if not (index < len(lines) and lines[index].lstrip().startswith("#")):
break

# print(repr(comment_block), repr(prefix), file=sys.stderr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug

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.

3 participants