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

Add a new knob and a change for inline comment alignment #1022

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

Conversation

lizawang
Copy link

@lizawang lizawang commented Aug 18, 2022

Hello yapf founders!
I want to share one knob and one change for the AlignTrailingComments feature.

The knob: ALIGN_NEWLINE_COMMENTS_WITH_INLINE_COMMENTS

This is a style option I added for when we do not want to align the newline comments with the inline comments in the same alignment block.

  • This option is used together with the SPACE_BEFORE_COMMENT = a list of numbers.

  • The default setting is True. When set to False, newline comments won't align with inline comments.

    For example, instead of

    if 1:
    encoding_bom = None
    if ( encoding.endswith( '-sig' ) or encoding.endswith( '-bom' ) ):
      encoding = encoding[ :-4 ]
      if ( encoding == 'utf-8' ):          # comment inline
                                           # comment newline
        encoding_bom = codecs.BOM_UTF16_BE
      else:
                                           #=========Comment Newlines=============================
                                           # I18N_MESSAGE | 210620850
                                           # de | BOM nicht unterstützt für Encoding '${enc}'.
                                           # en | BOM not supported for encoding '${enc}'.
                                           #======================================================
        result[ 'success' ] = False
        result[ 'error_code' ] = 210620850 # comment inline
    

    we want

    if 1:
    encoding_bom = None
    if ( encoding.endswith( '-sig' ) or encoding.endswith( '-bom' ) ):
      encoding = encoding[ :-4 ]
      if ( encoding == 'utf-8' ):          # comment inline
        # comment newline
        encoding_bom = codecs.BOM_UTF16_BE
      else:
        #=========Comment Newlines=============================
        # I18N_MESSAGE | 210620850
        # de | BOM nicht unterstützt für Encoding '${enc}'.
        # en | BOM not supported for encoding '${enc}'.
        #======================================================
        result[ 'success' ] = False
        result[ 'error_code' ] = 210620850 # comment inline
    
  • A problem rises when using this new knob, instead of remaining the spaces before the standalone newline comment inside a logical line:

def f():
  result = {
      "a": 1,
      # comment
      "abc": 2
  }

The comment is dedented to its parent level like:

def f():
  result = {
      "a": 1,
  # comment
      "abc": 2
  }

This is also fixed by adding one line in _GetNewlineColumn(self), restricting to set the spaces before the comment to 0 only when it’s not inside a logical line(aka with parent_level==0).

One change

Another change I added is when there is an object(dictionary, argument list, list, function call) with its entries on separate lines with its closing bracket also on a separate line, it starts a new alignment block.
For example:

func( 1 )               # Line 1
func( 2 )               # Line 2
d = {
    key1: value1,
    key2: value2,
    key3: value3,
}                       # Line 3
func( 3 )     # Line 4
func( 4 )     # line 5

Finally

  • The knob is added in style.py. The default setting is True.
  • The tests for the new knob and new alignment after the object with entries on separate lines are added in yapf_test.py. All yapftests are run and no test fail is caused by the changes.

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.

Thanks for the PR! Please run yapf over the changes. Also please update the CHANGELOG and README.rst files.

Comment on lines 941 to 942
# only when the commet is not inside an object(list, dictionary,
# function call)logical line that is in many output lines
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify this comment, please? I'm not sure what it's trying to suggest.

Copy link
Author

@lizawang lizawang Aug 30, 2022

Choose a reason for hiding this comment

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

Thanks for the replies. I will fix all the problems today. We know that a dictionary, a list and an argument list is one logical line no matter how long it is with its entries all output in separate lines. Here it means that only when the comment is not inside this kind of logical line, aka, the comment has parent level equal to 0, we set the spaces_required_before to 0. Because we don't want the comments inside a dictionary to align with its parent (the dictionary variable name). Examples are given in pull request README.

@@ -42,7 +42,6 @@ def SpliceComments(tree):
# This is a list because Python 2.x doesn't have 'nonlocal' :)
prev_leaf = [None]
_AnnotateIndents(tree)

Copy link
Member

Choose a reason for hiding this comment

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

Please retain this newline.

Comment on lines 281 to 282
# All trailing comments
# NOTE not including comments that appear on a line by themselves
Copy link
Member

Choose a reason for hiding this comment

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

Please reflow this comment. And place any "NOTE" comments at the end of the comment block.

@@ -309,7 +311,16 @@ def _AlignTrailingComments(final_lines):
line_content = ''
pc_line_lengths = []

#NOTE
Copy link
Member

Choose a reason for hiding this comment

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

Empty note.

for line_tok in this_line.tokens:

#NOTE if a line with inline comment is itself
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need a "NOTE" tag here (or in some other places).

Comment on lines 381 to 383
''' this is added when we don't want comments on newlines
to align with comments inline
'''
Copy link
Member

Choose a reason for hiding this comment

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

This should be a comment instead of multiline string.

@@ -26,7 +26,7 @@

from lib2to3.pgen2 import tokenize

from yapf.yapflib import errors
from yapf.yapflib import errors, reformatter
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this.

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.

2 participants