Issue 1155: Handle LHS and RHS values differently

[vemel]

Idea

black already has left_side_split function, but it is only used for function definitions. The idea is to extract LHS and RHS values from a string, split them separately and join them back to a final result.

I added logic for splitting LHS and RHS and updated SplitFunc to always be aware of a target line max length. The downside of this approach is that now if LHS can be rendered to a single line - it always does. I need an advice on how to handle it correctly. Probably, we should explode LHS line if RHS has unsplittable type ignore, but it is not always safe to do this.

Changes

• Removed line_str argument from is_line_short_enough function - not used
• Removed Line.is_collection_with_optional_trailing_comma method
• Removed Line.contains_unsplittable_type_ignore method
• Added Line.get_optional_trailing_comma_index method
• Added Line.get_leaf_index method
• Added Line._get_collection_comma_indexes method - helper for get_optional_trailing_comma_index
• Added Line.get_unsplittable_type_ignore method
• Added Line._get_assignment_index method - helper to split Line.get_left_hand_side and Line.get_right_hand_side
• Added Line._get_top_level_collection_indexes method - helper for Line.get_optional_trailing_comma_index
• Added Line.get_left_hand_side method - returns line with leaves and comment for the LHS value
• Added Line.get_left_right_sidemethod - returns line with leaves and comment for the RHS value and assignment
• Added Line.should_be_rendered_as_single_line method - check if line should not be exploded anf fits desired line length
• Added Line.is_short_enough method - potential replacement for is_line_short_enough function, but can handle multiline Line as well
• Added right_hand_split_with_omit function - replacement for rhs function that was created dynamically
• Added Line.get_source_line_numbers method to check unsplittable type ignore is on the same lines as LHS
• Added split_line_side function - split_line uses it for LHS and RHS
• All SplitFunc now have line_length and first_line_length arguments
• Updated unit tests

Output result changes

# LHS 1-item subscript output
Tuple[str,]
# RHS 1-item subscript output
MyType = Tuple[str,]

# 1-arg function definition with trailing comma output
def test(
    a,
):
    pass

# multi-arg function definition with trailing comma output
def test(
    a,
    b,
):
    pass

[JelleZijlstra]

As with your other PR, this is a very large change, which makes it difficult for me to find the time to give it a thorough review.

I noticed that there are significant changes in the formatting output for common cases. We generally avoid making such changes, so that codebases that are already Black-formatted don't see unnecessary change.

[vemel]

Makes sense. What do you think about leaving only part that explodes lines on trailing commas in nested collections, e.g.

def test(
    [
        1,
        2,
    ]
): ...

Then we need only changes in Line class.

[ambv]

@vemel, makes sense to leave only the case you show in your last comment.

[vemel]

Okay, I will prepare a new PR then.

[lorencarvalho]

Hi @ambv, @JelleZijlstra & @vemel,

I am very interested in this change, as I have been running black against some large-ish codebases and I keep running into changes like this:

--- test.py	2020-02-21 21:06:16.454911 +0000
+++ test.py	2020-02-21 21:06:17.331524 +0000
@@ -1,13 +1,5 @@
 def __init__(
-    self,
-    an: bool,
-    arbitrarily: str,
-    long_: int,
-    list_: Any,
-    of: str,
-    arguments: bool,
-    *args: Any,
-    **kwargs: Any,
+    self, an: bool, arbitrarily: str, long_: int, list_: Any, of: str, arguments: bool, *args: Any, **kwargs: Any
 ) -> None:
     pass

Even if the longer version fits within the line length, I do not want black to collapse the list of arguments, and I don't expect it to since there is a trailing comma (I believe the trailing comma rule only applies to collections that are not nested). This PR appears to solve my problem, but the comments here indicate that it's too radical of a change:

I noticed that there are significant changes in the formatting output for common cases. We generally avoid making such changes, so that codebases that are already Black-formatted don't see unnecessary change.

While I certainly understand this sentiment, I would argue that the current behavior is sub-optimal. Taking an intentionally broken apart collection (or call signature) and collapsing it seems to be optimizing for fewer vertical lines, but imo harms code readability and especially harms diff readability. In addition, having distinct behavior around how black interprets trailing commas feels counter-intuitive (e.g. if black treats collections with trailing commas one way, why does it treat call signatures with trailing commas differently?). I was also going to mention the bug where trailing commas are preserved after collapsing call signatures, but I believe that has fixed.

Is the implication here that once a style decision has been merged into black's codebase, it can never be changed in order to avoid unexpected changes in existing codebases that are formatted with black? Are there any precedents? Perhaps this sort of thing can be gated behind a major or minor version increment?

Thanks for reading!

[ambv]

This is resolved via other pull requests now. Thank you for the comprehensive contribution but we won't be able to merge this.