Insert newline between docstring and following own line comment #8216

konstin · 2023-10-25T13:44:10Z

Summary Previously, own line comment following after a docstring followed by newline(s) before the first content statement were treated as trailing on the docstring and we didn't insert a newline after the docstring as black would.

Before:

class ModuleBrowser:
    """Browse module classes and functions in IDLE."""
    # This class is also the base class for pathbrowser.PathBrowser.

    def __init__(self, master, path, *, _htest=False, _utest=False):
        pass

After:

class ModuleBrowser:
    """Browse module classes and functions in IDLE."""

    # This class is also the base class for pathbrowser.PathBrowser.

    def __init__(self, master, path, *, _htest=False, _utest=False):
        pass

I'm not entirely happy about hijacking handle_own_line_comment_between_statements, but i don't know a better spot to put it.

Fixes #7948

Test Plan Fixtures

MichaReiser

I like the solution

charliermarsh · 2023-10-26T01:57:26Z

crates/ruff_python_formatter/src/comments/placement.rs

+    //
+    //     def __init__(self, master, path, *, _htest=False, _utest=False):
+    //         pass
+    // ```


What if there's no statement in the body after the comment? Like:

class ModuleBrowser: """Browse....""" # Insert a newline above

Black inserts a newline there -- does this handle that case?

Looks like it doesn't support that case yet. How else could we approach it? 🤔

Fixed, manually inserting a newline seems to be the best option

charliermarsh · 2023-10-28T01:07:14Z

crates/ruff_python_formatter/src/statement/suite.rs

+                }
+            }
+
+            trailing_comments(node_comments.trailing).fmt(f)


This should only be applied to class docstrings, right? (Wouldn't it also apply to function docstrings as written?)

@konstin - Sorry to ping, did this get addressed? I don't see test cases for it.

Addressed in #8375

charliermarsh · 2023-10-28T01:07:21Z

crates/ruff_python_formatter/src/statement/suite.rs

+            )?;
+
+            // Comments after docstrings need a newline between the docstring and the comment, so we attach
+            // it as leading on the first statement after the docstring.


I think this comment is stale.

github-actions · 2023-10-30T13:26:53Z

PR Check Results

Ecosystem

✅ ecosystem check detected no format changes.

MichaReiser · 2023-10-31T08:27:53Z

It seems that this PR regressed the Black compatibility for django, home-assistan, transformers, and zullip. The index is unchanged, but each project has significantly more changed files:

Before

project	similarity index	total files	changed files
django	0.99984	2772	33
home-assistant	0.99963	10596	148
transformers	0.99967	2657	328
zulip	0.99970	1459	22

After

project	similarity index	total files	changed files
django	0.99984	2772	48
home-assistant	0.99963	10596	181
transformers	0.99967	2657	339
zulip	0.99970	1459	23

I'm not sure why this wasn't catched by the ecosystem check (CC: @zanieb) but we should make sure that this doesn't get released by either reverting the change, or following up with a follow up PR before the next release goes out.

Fixup for #8216 to not apply to function docstrings. Main before #8216: | project | similarity index | total files | changed files | |----------------|------------------:|------------------:|------------------:| | cpython | 0.75804 | 1799 | 1648 | | django | 0.99984 | 2772 | 33 | | home-assistant | 0.99963 | 10596 | 148 | | poetry | 0.99925 | 317 | 12 | | transformers | 0.99967 | 2657 | 328 | | twine | 1.00000 | 33 | 0 | | typeshed | 0.99980 | 3669 | 18 | | warehouse | 0.99977 | 654 | 13 | | zulip | 0.99970 | 1459 | 22 | main now: | project | similarity index | total files | changed files | |----------------|------------------:|------------------:|------------------:| | cpython | 0.75804 | 1799 | 1648 | | django | 0.99984 | 2772 | 48 | | home-assistant | 0.99963 | 10596 | 181 | | poetry | 0.99925 | 317 | 12 | | transformers | 0.99967 | 2657 | 339 | | twine | 1.00000 | 33 | 0 | | typeshed | 0.99980 | 3669 | 18 | | warehouse | 0.99977 | 654 | 13 | | zulip | 0.99970 | 1459 | 23 | PR: | project | similarity index | total files | changed files | |----------------|------------------:|------------------:|------------------:| | cpython | 0.75804 | 1799 | 1648 | | django | 0.99984 | 2772 | 33 | | home-assistant | 0.99963 | 10596 | 148 | | poetry | 0.99925 | 317 | 12 | | transformers | 0.99967 | 2657 | 328 | | twine | 1.00000 | 33 | 0 | | typeshed | 0.99980 | 3669 | 18 | | warehouse | 0.99977 | 654 | 13 | | zulip | 0.99970 | 1459 | 22 |

Previously, the ecosystem checks formatted with the baseline then formatted again with `--diff` to get the changed files. Now, the ecosystem checks support a new mode where we: - Format with the baseline - Commit the changes - Reset to the target ref - Format again - Check the diff from the baseline commit This effectively tests Ruff changes on unformatted code rather than changes in previously formatted code (unless, of course, the project is already using Ruff). While this mode is the new default, I've retained the old one for local checks. The mode can be toggled with `--format-comparison <type>`. Includes some more aggressive resetting of the GitHub repositories when cached. Here, I've also stubbed comparison modes in which `black` is used as the baseline. While these do nothing here, #8419 adds support. I tested this with the commit from #8216 and ecosystem changes appear https://gist.github.com/zanieb/a982ec8c392939043613267474471a6e

konstin added the formatter Related to the formatter label Oct 25, 2023

konstin requested a review from MichaReiser October 25, 2023 13:44

MichaReiser requested a review from charliermarsh October 26, 2023 00:26

MichaReiser approved these changes Oct 26, 2023

View reviewed changes

charliermarsh reviewed Oct 26, 2023

View reviewed changes

konstin mentioned this pull request Oct 27, 2023

Add newline after module docstrings in preview style #8283

Merged

charliermarsh reviewed Oct 28, 2023

View reviewed changes

konstin force-pushed the insert-newline-between-docstring-and-comment branch from 9b1698f to 532014b Compare October 30, 2023 13:06

Insert newline between docstring and following own line comment

4ff458a

konstin force-pushed the insert-newline-between-docstring-and-comment branch from 532014b to 4ff458a Compare October 30, 2023 13:07

Update stale comment

7a35a5a

konstin enabled auto-merge (squash) October 30, 2023 13:09

konstin merged commit b6c4074 into main Oct 30, 2023
16 checks passed

konstin deleted the insert-newline-between-docstring-and-comment branch October 30, 2023 13:18

konstin mentioned this pull request Oct 31, 2023

No newline after function docstrings #8375

Merged

zanieb mentioned this pull request Nov 1, 2023

Add new ecosystem comparison modes for the formatter #8416

Merged

miccal mentioned this pull request Nov 3, 2023

ruff 0.1.4 Homebrew/homebrew-core#153286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert newline between docstring and following own line comment #8216

Insert newline between docstring and following own line comment #8216

konstin commented Oct 25, 2023

MichaReiser left a comment

charliermarsh Oct 26, 2023 •

edited

Loading

charliermarsh Oct 26, 2023

konstin Oct 27, 2023

charliermarsh Oct 28, 2023

charliermarsh Oct 30, 2023

konstin Oct 31, 2023

charliermarsh Oct 28, 2023

github-actions bot commented Oct 30, 2023

MichaReiser commented Oct 31, 2023

Insert newline between docstring and following own line comment #8216

Insert newline between docstring and following own line comment #8216

Conversation

konstin commented Oct 25, 2023

MichaReiser left a comment

Choose a reason for hiding this comment

charliermarsh Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Oct 26, 2023

Choose a reason for hiding this comment

konstin Oct 27, 2023

Choose a reason for hiding this comment

charliermarsh Oct 28, 2023

Choose a reason for hiding this comment

charliermarsh Oct 30, 2023

Choose a reason for hiding this comment

konstin Oct 31, 2023

Choose a reason for hiding this comment

charliermarsh Oct 28, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2023

PR Check Results

Ecosystem

MichaReiser commented Oct 31, 2023

charliermarsh Oct 26, 2023 •

edited

Loading