-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Preview Style: Format module level docstring #9725
Preview Style: Format module level docstring #9725
Conversation
fc22e42
to
bef24c2
Compare
19c1343
to
2501c76
Compare
|
@MichaReiser I think this is ready for review. The only testing failure is happening because we are formatting module docstrings now and we remove the last new line character from docstrings after format(example). I'm not sure if we want to keep this or should I opt out of this for module docstrings to keep compatibility. Because this is not listed under intentional deviations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here look good but I think we have to look into why we trim the last line or this will be a very disruptive change.
@Glyphack I can take a look tomorrow morning except if you want to do it. I'm really curious what's causing this difference. That's not at all what I expected. |
Oh I think I have a suspision... The difference is that module level docstring have no indent... Some code in the docstring formatting must be relying on the indent to be present. |
@MichaReiser I am interested and looking into it. Will post my findings. Another thing I checked is that looks like black is not formatting module docstrings. source:
black output:
ruff output if we format module docstrings:
|
52d01bd
to
d51aa6a
Compare
d51aa6a
to
2cbafd2
Compare
7a13f96
to
c63bc47
Compare
c63bc47
to
935b59a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I suggest that we revert the changes in docstring.rs
and address the "collapse" issue separately because it isn't specific to module docstrings.
if is_module && trim_end.ends_with('\n') { | ||
hard_line_break().fmt(f)?; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bug in our codebock formatting or docstring formatting over all and is not specific to module formatting (CC: @BurntSushi).
For example:
class Test:
"""
Black's `Preview.module_docstring_newlines`
A code black to format
"""
Black formats this to
class Test:
"""
Black's `Preview.module_docstring_newlines`
A code black to format
"""
and Ruff formats it to
class Test:
"""
Black's `Preview.module_docstring_newlines`
A code black to format"""
Note how the last line gets collapsed.
The relevant logic is in
ruff/crates/ruff_python_formatter/src/string/docstring.rs
Lines 388 to 398 in 935b59a
let trim_end = line.line.trim_end(); | |
if trim_end.is_empty() { | |
return if line.is_last { | |
// If the doc string ends with ` """`, the last line is | |
// ` `, but we don't want to insert an empty line (but close | |
// the docstring). | |
Ok(()) | |
} else { | |
empty_line().fmt(self.f) | |
}; | |
} |
My understanding is that the last line should only be collapsed for docstrings with a single content line:
Collapse
class Test:
"""Black A code black to format
"""
Don't collapse for
class Test:
"""Black A code black to format
d
"""
Keep it collapsed when it was collapsed in the source code
class Test:
"""Black A code black to format
a"""
I suggest that we revert this change and address this issue in its own PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change just note that the current implementation would have a different formatting if applied twice in the following example. Just flagging it so if you want we can keep this PR open until that one gets fixed.
"""
A docstring
.. code-block:: python
button.on_event(ButtonClick, callback)
"""
Format once:
"""
A docstring
.. code-block:: python
button.on_event(ButtonClick, callback)
"""
Format twice:
"""
A docstring
.. code-block:: python
button.on_event(ButtonClick, callback)"""
935b59a
to
82f1d08
Compare
The preview format does nothing to the code block so it was misleading and you would think that the formatter did not format that docstring but it actually did. Just did not format the code block.
42637a2
to
f5ddfb0
Compare
Summary
This PR adds support for formatting module level docstrings as described in https://peps.python.org/pep-0257/.
Since black does not support formatting docstrings and this is not intentional we need to keep this rule in preview.
On the formatting style, we do one thing different than function and class docstrings which is keeping the trailing new line. This pattern is already used by python projects(example) and also docformat does it.
Fixes: #9701
Test Plan