Repository: google/yapf
Pull request #191 (2015-10-06 06:30:11)

Add DEDENT_CLOSING_BRACKETS
This allows for PEP8-compatible JSON-style scope organization that has the
following properties:

* produces smaller diffs
* uniformly treats all closing brackets, including function definitions,
  calls and data structures
* leaves more space on the line by solving the PEP8 violation known as
  "E125: continuation line with same indent as next logical line" without additional indentation necessary

## Summary of the changes in this diff
* this is an updated version of #187 which was closed by mistake
* adds `DEDENT_CLOSING_BRACKETS` (`False` by default)
* I added a "facebook" style that closely resembles what we endorse internally
* `_DetermineMustSplitAnnotation` is ran on imports, except clauses, function definitions and calls
* I bumped the version according to semver

## Changes since #187
### DEDENT_CLOSING_BRACKETS

When DEDENT_CLOSING_BRACKETS decides to put a break before the closing bracket, we are now forcing a break after the **opening** bracket as well. This is done both to have more horizontal space for the trailer and for consistency. By consistency I mean both the resulting code and internal consistency within YAPF which docstrings say in format_decision_state.py that we only want to split_before_closing_bracket if there was a newline after the opening one.

### pytree_unwrapper._DetermineMustSplitAnnotation
This worked only for lists, dicts and sets. I made it work for `import_as_names`, `testlist_gexp`, `arglist` and `typedargslist` as well. Again, consistency. This needed one change in your tests (first example in `testAlignClosingBracketWithVisualIndentation`) but I think the current formatting is better. If you don’t like this being the default, we can add a toggle for it.

### Tests
* added TestsForFBStyle in reformatter_test.py to ensure coverage

## Future design remarks
Those observations shouldn't block merging of this PR. They're rather opening the discussion about where do you want YAPF to go. 
### _DetermineMustSplitAnnotation feels arbitrary

Granted, the comma at the end or the presence of comments are nice heuristics. However, this produces vastly different results based only on the presence of one semantically insignificant character at the end of the container literal.

Instead, it would be clearer if we always preferred single-line lists when they fit. The high-level design would be like this:
* always prefer single-line formatting if it fits
* then prefer a single-line **element list** (argument list, etc.) if it fits
* otherwise, split one element per line

That’d be very easy to explain, especially seeing an example like this:
```python
# the following fits in one line
singleline(a, b, c)

# the following argslist fits in one line
singleline_separate_args(
  a, b, c
)

# there’s no way to fit the entire thing
# or its argslist in one line
multiline(
  a,
  b,
  c
)
```
Then the trailing comma becomes insignificant from YAPF’s perspective.

### Lack of idempotency
I noticed some of the reformattings are not idempotent, e.g. if you do multiple passes, the result will be mutating a couple of times. It will land at some equilibrium but it shows that some heuristics went a little too far (I suspect those that look at if there's a newline in the original source).
### Performance
There is a noticable performance hit because of _DetermineMustSplitAnnotation being run more often but no comprehensive benchmarks have been made.



Patch:
diff --git a/CHANGELOG b/CHANGELOG
index e9c2fc7..51a4382 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,10 @@
 # All notable changes to this project will be documented in this file.
 # This project adheres to [Semantic Versioning](http://semver.org/).
 
+## [0.4.0] UNRELEASED
+### Added
+- Support for dedenting closing brackets, "facebook" style.
+
 ## [0.3.2] 2015-10-04
 ### Fixed
 - Formatting of tokens after a multiline string didn't retain their horizontal
diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 7d31575..62e2a5f 100644
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -12,3 +12,4 @@
 Bill Wendling <morbo@google.com>
 Eli Bendersky <eliben@google.com>
 Sam Clegg <sbc@google.com>
+Łukasz Langa <ambv@fb.com>
diff --git a/README.rst b/README.rst
index 6187c23..28d9710 100644
--- a/README.rst
+++ b/README.rst
@@ -184,7 +184,7 @@ share several arguments which are described below:
 .. code-block:: python
 
     >>> from yapf.yapf_api import FormatCode  # reformat a string of code
-    
+
     >>> FormatCode("f ( a = 1, b = 2 )")
     'f(a=1, b=2)\n'
 
@@ -284,6 +284,11 @@ You can also disable formatting for a single literal like this:
         (9, 10, 11, 12),
     }  # yapf: disable
 
+To preserve the nice dedented closing brackets, use the
+``dedent_closing_brackets`` in your style. Note that in this case all
+brackets, including function definitions and calls, are going to use
+that style.  This provides consistency across the formatted codebase.
+
 Why Not Improve Existing Tools?
 -------------------------------
 
diff --git a/yapf/__init__.py b/yapf/__init__.py
index debd632..530dd72 100644
--- a/yapf/__init__.py
+++ b/yapf/__init__.py
@@ -38,7 +38,8 @@ from yapf.yapflib import py3compat
 from yapf.yapflib import style
 from yapf.yapflib import yapf_api
 
-__version__ = '0.3.2'
+
+__version__ = '0.4.0'
 
 
 def main(argv):
diff --git a/yapf/yapflib/pytree_unwrapper.py b/yapf/yapflib/pytree_unwrapper.py
index 5534e30..95c608d 100644
--- a/yapf/yapflib/pytree_unwrapper.py
+++ b/yapf/yapflib/pytree_unwrapper.py
@@ -202,6 +202,22 @@ class PyTreeUnwrapper(pytree_visitor.PyTreeVisitor):
     _DetermineMustSplitAnnotation(node)
     self.DefaultNodeVisit(node)
 
+  def Visit_import_as_names(self, node):  # pylint: disable=invalid-name
+    _DetermineMustSplitAnnotation(node)
+    self.DefaultNodeVisit(node)
+
+  def Visit_testlist_gexp(self, node):  # pylint: disable=invalid-name
+    _DetermineMustSplitAnnotation(node)
+    self.DefaultNodeVisit(node)
+
+  def Visit_arglist(self, node):  # pylint: disable=invalid-name
+    _DetermineMustSplitAnnotation(node)
+    self.DefaultNodeVisit(node)
+
+  def Visit_typedargslist(self, node):  # pylint: disable=invalid-name
+    _DetermineMustSplitAnnotation(node)
+    self.DefaultNodeVisit(node)
+
   def DefaultLeafVisit(self, leaf):
     """Default visitor for tree leaves.
 
diff --git a/yapf/yapflib/style.py b/yapf/yapflib/style.py
index dabf666..60fb1f5 100644
--- a/yapf/yapflib/style.py
+++ b/yapf/yapflib/style.py
@@ -69,6 +69,26 @@ _STYLE_HELP = dict(
                          # <------ this blank line
         def method():
           ..."""),
+    DEDENT_CLOSING_BRACKETS=("""\
+      Put closing brackets on a separate line, dedented, if the bracketed
+      expression can't fit in a single line. Applies to all kinds of brackets,
+      including function definitions and calls.
+
+      For example:
+
+      config = {
+        'key1': 'value1',
+        'key2': 'value2',
+      }        # <--- this bracket is dedented and on a separate line
+
+      time_series = self.remote_client.query_entity_counters(
+        entity='dev3246.region1',
+        key='dns.query_latency_tcp',
+        transform=Transformation.AVERAGE(window=timedelta(seconds=60)),
+        start_ts=now()-timedelta(days=3),
+        end_ts=now(),
+      )        # <--- this bracket is dedented and on a separate line
+      """),
     JOIN_MULTIPLE_LINES=
     "Join short lines into one line. E.g., single line 'if' statements.",
     SPACE_BETWEEN_ENDING_COMMA_AND_CLOSING_BRACKET=textwrap.dedent("""\
@@ -102,6 +122,7 @@ def CreatePEP8Style():
   return dict(
       ALIGN_CLOSING_BRACKET_WITH_VISUAL_INDENT=True,
       COLUMN_LIMIT=79,
+      DEDENT_CLOSING_BRACKETS=False,
       I18N_COMMENT='',
       I18N_FUNCTION_CALL='',
       INDENT_IF_EXPR_CONTINUATION=4,
@@ -143,10 +164,23 @@ def CreateChromiumStyle():
   return style
 
 
+def CreateFacebookStyle():
+  style = CreatePEP8Style()
+  style['ALIGN_CLOSING_BRACKET_WITH_VISUAL_INDENT'] = False
+  style['COLUMN_LIMIT'] = 80
+  style['DEDENT_CLOSING_BRACKETS'] = True
+  style['JOIN_MULTIPLE_LINES'] = False
+  style['SPACES_BEFORE_COMMENT'] = 2
+  style['SPLIT_PENALTY_AFTER_OPENING_BRACKET'] = 0
+  style['SPLIT_PENALTY_FOR_ADDED_LINE_SPLIT'] = 30
+  return style
+
+
 _STYLE_NAME_TO_FACTORY = dict(
     pep8=CreatePEP8Style,
     chromium=CreateChromiumStyle,
     google=CreateGoogleStyle,
+    facebook=CreateFacebookStyle,
 )  # yapf: disable
 
 
@@ -170,6 +204,7 @@ def _BoolConverter(s):
 _STYLE_OPTION_VALUE_CONVERTER = dict(
     ALIGN_CLOSING_BRACKET_WITH_VISUAL_INDENT=_BoolConverter,
     COLUMN_LIMIT=int,
+    DEDENT_CLOSING_BRACKETS=_BoolConverter,
     I18N_COMMENT=str,
     I18N_FUNCTION_CALL=_StringListConverter,
     INDENT_IF_EXPR_CONTINUATION=int,
diff --git a/yapf/yapflib/unwrapped_line.py b/yapf/yapflib/unwrapped_line.py
index 4273718..bdbf121 100644
--- a/yapf/yapflib/unwrapped_line.py
+++ b/yapf/yapflib/unwrapped_line.py
@@ -73,6 +73,9 @@ class UnwrappedLine(object):
           _SpaceRequiredBetween(prev_token, token)):
         token.spaces_required_before = 1
 
+      token.total_length = (
+          prev_length + len(token.value) + token.spaces_required_before)
+
       # The split penalty has to be computed before {must|can}_break_before,
       # because these may use it for their decision.
       token.split_penalty += _SplitPenalty(prev_token, token)
@@ -80,10 +83,6 @@ class UnwrappedLine(object):
       token.can_break_before = (token.must_break_before or
                                 _CanBreakBefore(prev_token, token))
 
-      token.total_length = (
-          prev_length + len(token.value) + token.spaces_required_before
-      )
-
       prev_length = token.total_length
       prev_token = token
 
@@ -313,6 +312,16 @@ def _MustBreakBefore(prev_token, cur_token):
     # reasonable assumption, because otherwise they should have written them
     # all on the same line, or with a '+'.
     return True
+  if style.Get('DEDENT_CLOSING_BRACKETS') and cur_token.ClosesScope():
+    opening = cur_token.matching_bracket
+    trailer_length = cur_token.total_length - opening.total_length
+    if (trailer_length > style.Get('COLUMN_LIMIT') or
+        cur_token.lineno != opening.lineno):
+      # Since we're already dedenting the closing bracket, let's put a newline
+      # after the opening one so that we have more horizontal space for the
+      # trailer.
+      opening.next_token.must_break_before = True
+      return True
   return pytree_utils.GetNodeAnnotation(cur_token.node,
                                         pytree_utils.Annotation.MUST_SPLIT,
                                         default=False)


Test patch:
diff --git a/yapftests/reformatter_test.py b/yapftests/reformatter_test.py
index 56d277d..3ac8e41 100644
--- a/yapftests/reformatter_test.py
+++ b/yapftests/reformatter_test.py
@@ -1747,7 +1747,7 @@ class BuganizerFixes(ReformatterTest):
             with io.open("/dev/null", "rb"):
               with io.open(os.path.join(aaaaa.bbbbb.ccccccccccc,
                                         DDDDDDDDDDDDDDD,
-                                        "eeeeeeeee ffffffffff",
+                                        "eeeeeeeee ffffffffff"
                                        ), "rb") as gggggggggggggggggggg:
                 print(gggggggggggggggggggg)
         """)
@@ -1757,7 +1757,7 @@ class BuganizerFixes(ReformatterTest):
           def bbbbbbbbbb(self):
             with io.open("/dev/null", "rb"):
               with io.open(os.path.join(aaaaa.bbbbb.ccccccccccc, DDDDDDDDDDDDDDD,
-                                        "eeeeeeeee ffffffffff",),
+                                        "eeeeeeeee ffffffffff"),
                            "rb") as gggggggggggggggggggg:
                 print(gggggggggggggggggggg)
         """)
@@ -2146,12 +2146,13 @@ class TestsForPEP8Style(ReformatterTest):
   def testAlignClosingBracketWithVisualIndentation(self):
     unformatted_code = textwrap.dedent("""\
         TEST_LIST = ('foo', 'bar',  # first comment
-                     'baz',  # second comment
+                     'baz'  # second comment
                     )
         """)
     expected_formatted_code = textwrap.dedent("""\
-        TEST_LIST = ('foo', 'bar',  # first comment
-                     'baz',  # second comment
+        TEST_LIST = ('foo',
+                     'bar',  # first comment
+                     'baz'  # second comment
                      )
         """)
     uwlines = _ParseAndUnwrap(unformatted_code)
@@ -2332,6 +2333,149 @@ class TestsForPython3Code(ReformatterTest):
     self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
 
 
+class TestsForFBStyle(ReformatterTest):
+
+  @classmethod
+  def setUpClass(cls):
+    style.SetGlobalStyle(style.CreateFacebookStyle())
+
+  def testNoNeedForLineBreaks(self):
+    unformatted_code = textwrap.dedent("""\
+        def overly_long_function_name(
+          just_one_arg, **kwargs):
+          pass
+        """)
+    expected_formatted_code = textwrap.dedent("""\
+        def overly_long_function_name(just_one_arg, **kwargs):
+            pass
+        """)
+    uwlines = _ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+
+  def testDedentClosingBracket(self):
+    unformatted_code = textwrap.dedent("""\
+        def overly_long_function_name(
+          first_argument_on_the_same_line,
+          second_argument_makes_the_line_too_long):
+          pass
+        """)
+    expected_formatted_code = textwrap.dedent("""\
+        def overly_long_function_name(
+            first_argument_on_the_same_line, \
+second_argument_makes_the_line_too_long
+        ):
+            pass
+        """)
+    uwlines = _ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+
+  def testBreakAfterOpeningBracketIfContentsTooBig(self):
+    unformatted_code = textwrap.dedent("""\
+        def overly_long_function_name(a, b, c, d, e, f, g, h, i, j, k, l, m,
+          n, o, p, q, r, s, t, u, v, w, x, y, z):
+          pass
+        """)
+    expected_formatted_code = textwrap.dedent("""\
+        def overly_long_function_name(
+            a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, \
+v, w, x, y, z
+        ):
+            pass
+        """)
+    uwlines = _ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+
+  def testDedentClosingBracketWithComments(self):
+    unformatted_code = textwrap.dedent("""\
+        def overly_long_function_name(
+          # comment about the first argument
+          first_argument_with_a_very_long_name_or_so,
+          # comment about the second argument
+          second_argument_makes_the_line_too_long):
+          pass
+        """)
+    expected_formatted_code = textwrap.dedent("""\
+        def overly_long_function_name(
+            # comment about the first argument
+            first_argument_with_a_very_long_name_or_so,
+            # comment about the second argument
+            second_argument_makes_the_line_too_long
+        ):
+            pass
+        """)
+    uwlines = _ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(expected_formatted_code, reformatter.Reformat(uwlines))
+
+  def testDedentImportAsNames(self):
+    unformatted_code = textwrap.dedent("""\
+    from module import (
+        internal_function as function,
+        SOME_CONSTANT_NUMBER1,
+        SOME_CONSTANT_NUMBER2,
+        SOME_CONSTANT_NUMBER3,
+    )
+    """)
+    uwlines = _ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(unformatted_code, reformatter.Reformat(uwlines))
+
+  def testDedentTestListGexp(self):
+    # TODO(ambv): Arguably _DetermineMustSplitAnnotation shouldn't enforce
+    # breaks only on the basis of a trailing comma if the entire thing fits
+    # in a single line.
+    unformatted_code = textwrap.dedent("""\
+    try:
+        pass
+    except (
+        IOError, OSError, LookupError, RuntimeError, OverflowError
+    ) as exception:
+        pass
+
+    try:
+        pass
+    except (
+        IOError,
+        OSError,
+        LookupError,
+        RuntimeError,
+        OverflowError,
+    ) as exception:
+        pass
+    """)
+    uwlines = _ParseAndUnwrap(unformatted_code)
+    self.assertCodeEqual(unformatted_code, reformatter.Reformat(uwlines))
+
+  def testBrokenIdempotency(self):
+    # TODO(ambv): The following behaviour should be fixed.
+    pass0_code = textwrap.dedent("""\
+    try:
+        pass
+    except (IOError, OSError, LookupError, RuntimeError, OverflowError\
+) as exception:
+        pass
+    """)
+    pass1_code = textwrap.dedent("""\
+    try:
+        pass
+    except (IOError, OSError, LookupError, RuntimeError, OverflowError
+           ) as exception:
+        pass
+    """)
+    uwlines = _ParseAndUnwrap(pass0_code)
+    self.assertCodeEqual(pass1_code, reformatter.Reformat(uwlines))
+
+    pass2_code = textwrap.dedent("""\
+    try:
+        pass
+    except (
+        IOError, OSError, LookupError, RuntimeError, OverflowError
+    ) as exception:
+        pass
+    """)
+    uwlines = _ParseAndUnwrap(pass1_code)
+    self.assertCodeEqual(pass2_code, reformatter.Reformat(uwlines))
+
+
+
 def _ParseAndUnwrap(code, dumptree=False):
   """Produces unwrapped lines from the given code.
 
diff --git a/yapftests/style_test.py b/yapftests/style_test.py
index 25064eb..cb1921a 100644
--- a/yapftests/style_test.py
+++ b/yapftests/style_test.py
@@ -56,6 +56,12 @@ def _LooksLikePEP8Style(cfg):
           not cfg['BLANK_LINE_BEFORE_NESTED_CLASS_OR_DEF'])
 
 
+def _LooksLikeFacebookStyle(cfg):
+  return (
+      cfg['INDENT_WIDTH'] == 4 and cfg['DEDENT_CLOSING_BRACKETS']
+  )
+
+
 class PredefinedStylesByNameTest(unittest.TestCase):
 
   def testDefault(self):
@@ -73,6 +79,11 @@ class PredefinedStylesByNameTest(unittest.TestCase):
       cfg = style.CreateStyleFromConfig(pep8_name)
       self.assertTrue(_LooksLikePEP8Style(cfg))
 
+  def testFacebookByName(self):
+    for fb_name in ('facebook', 'FACEBOOK', 'Facebook'):
+      cfg = style.CreateStyleFromConfig(fb_name)
+      self.assertTrue(_LooksLikeFacebookStyle(cfg))
+
 
 @contextlib.contextmanager
 def _TempFileContents(dirname, contents):
@@ -135,6 +146,17 @@ class StyleFromFileTest(unittest.TestCase):
       self.assertTrue(_LooksLikeGoogleStyle(cfg))
       self.assertEqual(cfg['CONTINUATION_INDENT_WIDTH'], 20)
 
+  def testDefaultBasedOnFacebookStyle(self):
+    cfg = textwrap.dedent('''\
+        [style]
+        based_on_style = facebook
+        continuation_indent_width = 20
+        ''')
+    with _TempFileContents(self.test_tmpdir, cfg) as f:
+      cfg = style.CreateStyleFromConfig(f.name)
+      self.assertTrue(_LooksLikeFacebookStyle(cfg))
+      self.assertEqual(cfg['CONTINUATION_INDENT_WIDTH'], 20)
+
   def testBoolOptionValue(self):
     cfg = textwrap.dedent('''\
         [style]
diff --git a/yapftests/yapf_test.py b/yapftests/yapf_test.py
index d33dfc4..9afccc1 100644
--- a/yapftests/yapf_test.py
+++ b/yapftests/yapf_test.py
@@ -428,6 +428,9 @@ class CommandLineTest(unittest.TestCase):
             if (xxxxxxxxxxxx.yyyyyyyy(zzzzzzzzzzzzz[0]) == 'aaaaaaaaaaa' and xxxxxxxxxxxx.yyyyyyyy(zzzzzzzzzzzzz[0].mmmmmmmm[0]) == 'bbbbbbb'):
                 pass
         """)
+    # TODO(ambv): the `expected_formatted_code` here is not PEP8 compliant,
+    # raising "E129 visually indented line with same indent as next logical
+    # line" with flake8.
     self.assertYapfReformats(unformatted_code, expected_formatted_code,
                              extra_options=['--lines', '1-2'])
 
@@ -823,6 +826,55 @@ class CommandLineTest(unittest.TestCase):
         """)
     self.assertYapfReformats(unformatted_code, expected_formatted_code,
                              extra_options=['--lines', '1-1'])
+  def testDedentClosingBracket(self):
+    # no line-break on the first argument, not dedenting closing brackets
+    unformatted_code = textwrap.dedent(u"""\
+      def overly_long_function_name(first_argument_on_the_same_line,
+      second_argument_makes_the_line_too_long):
+        pass
+    """)
+    expected_formatted_code = textwrap.dedent(u"""\
+      def overly_long_function_name(first_argument_on_the_same_line,
+                                    second_argument_makes_the_line_too_long):
+          pass
+    """)
+    self.assertYapfReformats(unformatted_code, expected_formatted_code,
+                             extra_options=['--style=pep8'])
+
+    # TODO(ambv): currently the following produces the closing bracket on a new
+    # line but indented to the opening bracket which is the worst of both
+    # worlds. Expected behaviour would be to format as --style=pep8 does in
+    # this case.
+    # self.assertYapfReformats(unformatted_code, expected_formatted_code,
+    #                          extra_options=['--style=facebook'])
+
+    # line-break before the first argument, dedenting closing brackets if set
+    unformatted_code = textwrap.dedent(u"""\
+      def overly_long_function_name(
+        first_argument_on_the_same_line,
+        second_argument_makes_the_line_too_long):
+        pass
+    """)
+    expected_formatted_pep8_code = textwrap.dedent(u"""\
+      def overly_long_function_name(
+              first_argument_on_the_same_line, \
+second_argument_makes_the_line_too_long):
+          pass
+    """)
+    expected_formatted_fb_code = textwrap.dedent(u"""\
+      def overly_long_function_name(
+          first_argument_on_the_same_line, second_argument_makes_the_line_too_long
+      ):
+          pass
+    """)
+    self.assertYapfReformats(unformatted_code, expected_formatted_fb_code,
+                             extra_options=['--style=facebook'])
+    # TODO(ambv): currently the following produces code that is not PEP8
+    # compliant, raising "E125 continuation line with same indent as next
+    # logical line" with flake8. Expected behaviour for PEP8 would be to use
+    # double-indentation here.
+    # self.assertYapfReformats(unformatted_code, expected_formatted_pep8_code,
+    #                          extra_options=['--style=pep8'])
 
 
 class BadInputTest(unittest.TestCase):