ruff icon indicating copy to clipboard operation
ruff copied to clipboard

[Python Formatter] Preserve empty lines

Open robincaloudis opened this issue 1 year ago • 5 comments

Summary

https://github.com/astral-sh/ruff/issues/9958 reported that code like

b = """
    This looks like a docstring but is not in a notebook because notebooks can't be imported as a module.
    Ruff should leave it as is
""";


a = "another normal string"

gets formatted as

b = """
    This looks like a docstring but is not in a notebook because notebooks can't be imported as a module.
    Ruff should leave it as is
""";
a = "another normal string"

This is unexpected and the expected behaviour of the formatter should preserve the empty lines between b and a.

What

  • Modified lines_after such that it respects the semicolon. In my opinion, the extension makes a lot of sense in this function as it should not break counting of lines due to a semicolon
  • Test call sides
  • I intentionally did not modify lines_before as alone standing ;'s are not allowed by the Python syntax anyway, e.g.
    b = """
        This looks like a docstring but is not in a notebook because notebooks can't be imported as a module.
        Ruff should leave it as is
    """;
    
    ;
    a = "another normal string"
    

Test Plan

  • Correct tests
  • Add tests

Closes https://github.com/astral-sh/ruff/issues/9958.

robincaloudis avatar Mar 27 '24 22:03 robincaloudis

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

github-actions[bot] avatar Mar 27 '24 22:03 github-actions[bot]

Some formatting has changed in the ruff-ecosystem. Seems like the backslash("\") is not recognized correctly with this change. Probably lines_after_ignoring_end_of_line_trivia needs to be adapted such that it interprets SimpleTokenKind::Continuation correctly. I will look into that.

robincaloudis avatar Mar 28 '24 08:03 robincaloudis

Thank you for working on this! I agree with Micha that blank lines are hard to get right even if it seems to be working. Even if the implementation work in isolation, it breaks when mixed with other things (speaking from personal experience ;)). As suggested, testing this out thoroughly would be really helpful for us to evaluate the change.

dhruvmanila avatar Mar 28 '24 09:03 dhruvmanila

I'm surprised this never occurs in the current ecosystem checks. Is there a project that uses this syntax that we could test on for a real world case?

zanieb avatar Mar 29 '24 03:03 zanieb

Hi @dhruvmanila and @MichaReiser, thank you both for your review. I agree. I rethought my current approach of extending lines_after_ignoring_end_of_line_trivia and decided to extend lines_after instead. Note that I intentionally did not extend lines_before. Find details in the PR description. This new approach should not mess up any corner cases, as it does not touches the current combination of lines_after and lines_before. @MichaReiser, do you mind to re-review? Thank you.

robincaloudis avatar Apr 04 '24 19:04 robincaloudis