Skip to content
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

Fix placement of inline parameter comments #13379

Merged
merged 3 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -458,3 +458,108 @@ def foo(x: S) -> S: ...

@decorator # comment
def foo(x: S) -> S: ...


# Regression tests for https://github.com/astral-sh/ruff/issues/13369
def foo(
arg: ( # comment with non-return annotation
int
# comment with non-return annotation
),
):
pass


def foo(
arg: ( # comment with non-return annotation
int
| range
| memoryview
# comment with non-return annotation
),
):
pass

def foo(arg: (
int
# only after
)):
pass

# Asserts that "incorrectly" placed comments don't *move* by fixing https://github.com/astral-sh/ruff/issues/13369
def foo(
# comment with non-return annotation
# comment with non-return annotation
arg: (int),
):
pass


# Comments between *args and **kwargs
def args_no_type_annotation(*
# comment
args): pass

def args_type_annotation(*
# comment
args: int): pass

def args_trailing_end_of_line_comment(* # comment
args): pass

def args_blank_line_comment(*

# comment

args): pass

def args_with_leading_parameter_comment(
# What comes next are arguments
*
# with an inline comment
args): pass


def kargs_no_type_annotation(**
# comment
kwargs): pass

def kwargs_type_annotation(**
# comment
kwargs: int): pass


def args_many_comments(
# before
*
# between * and name
args # trailing args
# after name
): pass


def args_many_comments_with_type_annotation(
# before
*
# between * and name
args # trailing args
# before colon
: # after colon
# before type
int # trailing type
# after type
): pass



def args_with_type_annotations_no_after_colon_comment(
# before
*
# between * and name
args # trailing args
# before colon
:
# before type
int # trailing type
# after type
): pass
52 changes: 41 additions & 11 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ use std::cmp::Ordering;

use ast::helpers::comment_indentation_after;
use ruff_python_ast::whitespace::indentation;
use ruff_python_ast::{self as ast, AnyNodeRef, Comprehension, Expr, ModModule, Parameters};
use ruff_python_ast::{
self as ast, AnyNodeRef, Comprehension, Expr, ModModule, Parameter, Parameters,
};
use ruff_python_trivia::{
find_only_token_in_range, indentation_at_offset, BackwardsTokenizer, CommentRanges,
SimpleToken, SimpleTokenKind, SimpleTokenizer,
find_only_token_in_range, first_non_trivia_token, indentation_at_offset, BackwardsTokenizer,
CommentRanges, SimpleToken, SimpleTokenKind, SimpleTokenizer,
};
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextLen, TextRange};
Expand Down Expand Up @@ -202,14 +204,7 @@ fn handle_enclosed_comment<'a>(
}
})
}
AnyNodeRef::Parameter(parameter) => {
// E.g. a comment between the `*` or `**` and the parameter name.
if comment.preceding_node().is_none() || comment.following_node().is_none() {
CommentPlacement::leading(parameter, comment)
} else {
CommentPlacement::Default(comment)
}
}
AnyNodeRef::Parameter(parameter) => handle_parameter_comment(comment, parameter, locator),
AnyNodeRef::Arguments(_) | AnyNodeRef::TypeParams(_) | AnyNodeRef::PatternArguments(_) => {
handle_bracketed_end_of_line_comment(comment, locator)
}
Expand Down Expand Up @@ -760,6 +755,41 @@ fn handle_parameters_separator_comment<'a>(
CommentPlacement::Default(comment)
}

/// Associate comments that come before the `:` starting the type annotation or before the
/// parameter's name for unannotated parameters as leading parameter-comments.
///
/// The parameter's name isn't a node to which comments can be associated.
/// That's why we pull out all comments that come before the expression name or the type annotation
/// and make them leading parameter comments. For example:
/// * `* # comment\nargs`
/// * `arg # comment\n : int`
///
/// Associate comments with the type annotation when possible.
fn handle_parameter_comment<'a>(
comment: DecoratedComment<'a>,
parameter: &'a Parameter,
locator: &Locator,
) -> CommentPlacement<'a> {
if parameter.annotation.as_deref().is_some() {
let colon = first_non_trivia_token(parameter.name.end(), locator.contents()).expect(
"A annotated parameter should have a colon following its name when it is valid syntax.",
);

assert_eq!(colon.kind(), SimpleTokenKind::Colon);

if comment.start() < colon.start() {
// The comment is before the colon, pull it out and make it a leading comment of the parameter.
CommentPlacement::leading(parameter, comment)
} else {
CommentPlacement::Default(comment)
}
} else if comment.start() < parameter.name.start() {
CommentPlacement::leading(parameter, comment)
} else {
CommentPlacement::Default(comment)
}
}

/// Handles comments between the left side and the operator of a binary expression (trailing comments of the left),
/// and trailing end-of-line comments that are on the same line as the operator.
///
Expand Down
23 changes: 18 additions & 5 deletions crates/ruff_python_formatter/src/other/parameter.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use ruff_formatter::write;
use ruff_python_ast::Parameter;

use crate::expression::parentheses::is_expression_parenthesized;
use crate::prelude::*;
use ruff_python_ast::Parameter;

#[derive(Default)]
pub struct FormatParameter;
Expand All @@ -16,8 +15,22 @@ impl FormatNodeRule<Parameter> for FormatParameter {

name.format().fmt(f)?;

if let Some(annotation) = annotation {
write!(f, [token(":"), space(), annotation.format()])?;
if let Some(annotation) = annotation.as_deref() {
token(":").fmt(f)?;

if f.context().comments().has_leading(annotation)
&& !is_expression_parenthesized(
annotation.into(),
f.context().comments().ranges(),
f.context().source(),
)
{
hard_line_break().fmt(f)?;
} else {
space().fmt(f)?;
}

annotation.format().fmt(f)?;
}

Ok(())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,53 +142,29 @@ variable: (
): ...


@@ -143,34 +141,31 @@


def foo(
- arg: ( # comment with non-return annotation
- int
- # comment with non-return annotation
- ),
+ # comment with non-return annotation
+ # comment with non-return annotation
+ arg: (int),
):
pass

@@ -153,16 +151,18 @@

def foo(
- arg: ( # comment with non-return annotation
arg: ( # comment with non-return annotation
- int
- | range
- | memoryview
- # comment with non-return annotation
- ),
+ # comment with non-return annotation
+ # comment with non-return annotation
+ arg: (int | range | memoryview),
+ int | range | memoryview
# comment with non-return annotation
),
):
pass


-def foo(arg: int): # only before
+def foo(
+ # only before
+ arg: (int),
+ arg: ( # only before
+ int
+ ),
+):
pass


def foo(
- arg: (
- int
- # only after
- ),
+ # only after
+ arg: (int),
):
pass

```

## Ruff Output
Expand Down Expand Up @@ -337,31 +313,36 @@ def foo() -> (


def foo(
# comment with non-return annotation
# comment with non-return annotation
arg: (int),
arg: ( # comment with non-return annotation
int
# comment with non-return annotation
),
):
pass


def foo(
# comment with non-return annotation
# comment with non-return annotation
arg: (int | range | memoryview),
arg: ( # comment with non-return annotation
int | range | memoryview
# comment with non-return annotation
),
):
pass


def foo(
# only before
arg: (int),
arg: ( # only before
int
),
):
pass


def foo(
# only after
arg: (int),
arg: (
int
# only after
),
):
pass

Expand Down
Loading
Loading