Skip to content

Commit 827d8ae

Browse files
authored
Allow newlines after function headers without docstrings (#21110)
Summary -- This is a first step toward fixing #9745. After reviewing our open issues and several Black issues and PRs, I personally found the function case the most compelling, especially with very long argument lists: ```py def func( self, arg1: int, arg2: bool, arg3: bool, arg4: float, arg5: bool, ) -> tuple[...]: if arg2 and arg3: raise ValueError ``` or many annotations: ```py def function( self, data: torch.Tensor | tuple[torch.Tensor, ...], other_argument: int ) -> torch.Tensor | tuple[torch.Tensor, ...]: do_something(data) return something ``` I think docstrings help the situation substantially both because syntax highlighting will usually give a very clear separation between the annotations and the docstring and because we already allow a blank line _after_ the docstring: ```py def function( self, data: torch.Tensor | tuple[torch.Tensor, ...], other_argument: int ) -> torch.Tensor | tuple[torch.Tensor, ...]: """ A function doing something. And a longer description of the things it does. """ do_something(data) return something ``` There are still other comments on #9745, such as [this one] with 9 upvotes, where users specifically request blank lines in all block types, or at least including conditionals and loops. I'm sympathetic to that case as well, even if personally I don't find an [example] like this: ```py if blah: # Do some stuff that is logically related data = get_data() # Do some different stuff that is logically related results = calculate_results() return results ``` to be much more readable than: ```py if blah: # Do some stuff that is logically related data = get_data() # Do some different stuff that is logically related results = calculate_results() return results ``` I'm probably just used to the latter from the formatters I've used, but I do prefer it. I also think that functions are the least susceptible to the accidental introduction of a newline after refactoring described in Micha's [comment] on #8893. I actually considered further restricting this change to functions with multiline headers. I don't think very short functions like: ```py def foo(): return 1 ``` benefit nearly as much from the allowed newline, but I just went with any function without a docstring for now. I guess a marginal case like: ```py def foo(a_long_parameter: ALongType, b_long_parameter: BLongType) -> CLongType: return 1 ``` might be a good argument for not restricting it. I caused a couple of syntax errors before adding special handling for the ellipsis-only case, so I suspect that there are some other interesting edge cases that may need to be handled better. Test Plan -- Existing tests, plus a few simple new ones. As noted above, I suspect that we may need a few more for edge cases I haven't considered. [this one]: #9745 (comment) [example]: psf/black#902 (comment) [comment]: #8893 (comment)
1 parent 1734ddf commit 827d8ae

File tree

7 files changed

+460
-28
lines changed

7 files changed

+460
-28
lines changed

crates/ruff_python_formatter/resources/test/fixtures/ruff/newlines.py

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,3 +335,96 @@ def overload4():
335335
# trailing comment
336336

337337
def overload4(a: int): ...
338+
339+
340+
# In preview, we preserve these newlines at the start of functions:
341+
def preserved1():
342+
343+
return 1
344+
345+
def preserved2():
346+
347+
pass
348+
349+
def preserved3():
350+
351+
def inner(): ...
352+
353+
def preserved4():
354+
355+
def inner():
356+
print("with a body")
357+
return 1
358+
359+
return 2
360+
361+
def preserved5():
362+
363+
...
364+
# trailing comment prevents collapsing the stub
365+
366+
367+
def preserved6():
368+
369+
# Comment
370+
371+
return 1
372+
373+
374+
def preserved7():
375+
376+
# comment
377+
# another line
378+
# and a third
379+
380+
return 0
381+
382+
383+
def preserved8(): # this also prevents collapsing the stub
384+
385+
...
386+
387+
388+
# But we still discard these newlines:
389+
def removed1():
390+
391+
"Docstring"
392+
393+
return 1
394+
395+
396+
def removed2():
397+
398+
...
399+
400+
401+
def removed3():
402+
403+
... # trailing same-line comment does not prevent collapsing the stub
404+
405+
406+
# And we discard empty lines after the first:
407+
def partially_preserved1():
408+
409+
410+
return 1
411+
412+
413+
# We only preserve blank lines, not add new ones
414+
def untouched1():
415+
# comment
416+
417+
return 0
418+
419+
420+
def untouched2():
421+
# comment
422+
return 0
423+
424+
425+
def untouched3():
426+
# comment
427+
# another line
428+
# and a third
429+
430+
return 0

crates/ruff_python_formatter/resources/test/fixtures/ruff/range_formatting/indent.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,3 +61,9 @@ def test6 ():
6161
print("Format" )
6262
print(3 + 4)<RANGE_END>
6363
print("Format to fix indentation" )
64+
65+
66+
def test7 ():
67+
<RANGE_START>print("Format" )
68+
print(3 + 4)<RANGE_END>
69+
print("Format to fix indentation" )

crates/ruff_python_formatter/src/preview.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,3 +36,10 @@ pub(crate) const fn is_remove_parens_around_except_types_enabled(
3636
) -> bool {
3737
context.is_preview()
3838
}
39+
40+
/// Returns `true` if the
41+
/// [`allow_newline_after_block_open`](https://github.com/astral-sh/ruff/pull/21110) preview style
42+
/// is enabled.
43+
pub(crate) const fn is_allow_newline_after_block_open_enabled(context: &PyFormatContext) -> bool {
44+
context.is_preview()
45+
}

crates/ruff_python_formatter/src/statement/clause.rs

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer};
88
use ruff_text_size::{Ranged, TextRange, TextSize};
99

1010
use crate::comments::{SourceComment, leading_alternate_branch_comments, trailing_comments};
11-
use crate::statement::suite::{SuiteKind, contains_only_an_ellipsis};
11+
use crate::statement::suite::{SuiteKind, as_only_an_ellipsis};
1212
use crate::verbatim::write_suppressed_clause_header;
1313
use crate::{has_skip_comment, prelude::*};
1414

@@ -449,17 +449,10 @@ impl Format<PyFormatContext<'_>> for FormatClauseBody<'_> {
449449
|| matches!(self.kind, SuiteKind::Function | SuiteKind::Class);
450450

451451
if should_collapse_stub
452-
&& contains_only_an_ellipsis(self.body, f.context().comments())
452+
&& let Some(ellipsis) = as_only_an_ellipsis(self.body, f.context().comments())
453453
&& self.trailing_comments.is_empty()
454454
{
455-
write!(
456-
f,
457-
[
458-
space(),
459-
self.body.format().with_options(self.kind),
460-
hard_line_break()
461-
]
462-
)
455+
write!(f, [space(), ellipsis.format(), hard_line_break()])
463456
} else {
464457
write!(
465458
f,

crates/ruff_python_formatter/src/statement/suite.rs

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ use crate::comments::{
1313
use crate::context::{NodeLevel, TopLevelStatementPosition, WithIndentLevel, WithNodeLevel};
1414
use crate::other::string_literal::StringLiteralKind;
1515
use crate::prelude::*;
16-
use crate::preview::is_blank_line_before_decorated_class_in_stub_enabled;
16+
use crate::preview::{
17+
is_allow_newline_after_block_open_enabled, is_blank_line_before_decorated_class_in_stub_enabled,
18+
};
1719
use crate::statement::stmt_expr::FormatStmtExpr;
1820
use crate::verbatim::{
1921
suppressed_node, write_suppressed_statements_starting_with_leading_comment,
@@ -169,6 +171,22 @@ impl FormatRule<Suite, PyFormatContext<'_>> for FormatSuite {
169171
false,
170172
)
171173
} else {
174+
// Allow an empty line after a function header in preview, if the function has no
175+
// docstring and no initial comment.
176+
let allow_newline_after_block_open =
177+
is_allow_newline_after_block_open_enabled(f.context())
178+
&& matches!(self.kind, SuiteKind::Function)
179+
&& matches!(first, SuiteChildStatement::Other(_));
180+
181+
let start = comments
182+
.leading(first)
183+
.first()
184+
.map_or_else(|| first.start(), Ranged::start);
185+
186+
if allow_newline_after_block_open && lines_before(start, f.context().source()) > 1 {
187+
empty_line().fmt(f)?;
188+
}
189+
172190
first.fmt(f)?;
173191

174192
let empty_line_after_docstring = if matches!(first, SuiteChildStatement::Docstring(_))
@@ -218,7 +236,7 @@ impl FormatRule<Suite, PyFormatContext<'_>> for FormatSuite {
218236
)?;
219237
} else {
220238
// Preserve empty lines after a stub implementation but don't insert a new one if there isn't any present in the source.
221-
// This is useful when having multiple function overloads that should be grouped to getter by omitting new lines between them.
239+
// This is useful when having multiple function overloads that should be grouped together by omitting new lines between them.
222240
let is_preceding_stub_function_without_empty_line = following
223241
.is_function_def_stmt()
224242
&& preceding
@@ -728,17 +746,21 @@ fn stub_suite_can_omit_empty_line(preceding: &Stmt, following: &Stmt, f: &PyForm
728746

729747
/// Returns `true` if a function or class body contains only an ellipsis with no comments.
730748
pub(crate) fn contains_only_an_ellipsis(body: &[Stmt], comments: &Comments) -> bool {
731-
match body {
732-
[Stmt::Expr(ast::StmtExpr { value, .. })] => {
733-
let [node] = body else {
734-
return false;
735-
};
736-
value.is_ellipsis_literal_expr()
737-
&& !comments.has_leading(node)
738-
&& !comments.has_trailing_own_line(node)
739-
}
740-
_ => false,
749+
as_only_an_ellipsis(body, comments).is_some()
750+
}
751+
752+
/// Returns `Some(Stmt::Ellipsis)` if a function or class body contains only an ellipsis with no
753+
/// comments.
754+
pub(crate) fn as_only_an_ellipsis<'a>(body: &'a [Stmt], comments: &Comments) -> Option<&'a Stmt> {
755+
if let [node @ Stmt::Expr(ast::StmtExpr { value, .. })] = body
756+
&& value.is_ellipsis_literal_expr()
757+
&& !comments.has_leading(node)
758+
&& !comments.has_trailing_own_line(node)
759+
{
760+
return Some(node);
741761
}
762+
763+
None
742764
}
743765

744766
/// Returns `true` if a [`Stmt`] is a class or function definition.

0 commit comments

Comments
 (0)