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 ArgWithDefault comments handling #5204

Merged
merged 1 commit into from
Jun 20, 2023
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
12 changes: 6 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ toml = { version = "0.7.2" }
# v0.0.1
libcst = { git = "https://github.com/charliermarsh/LibCST", rev = "80e4c1399f95e5beb532fdd1e209ad2dbb470438" }
# v0.0.3
ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "ed3b4eb72b6e497bbdb4d19dec6621074d724130" }
ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "08ebbe40d7776cac6e3ba66277d435056f2b8dca" }
# v0.0.3
rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "ed3b4eb72b6e497bbdb4d19dec6621074d724130" , default-features = false, features = ["all-nodes-with-ranges", "num-bigint"]}
rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "08ebbe40d7776cac6e3ba66277d435056f2b8dca" , default-features = false, features = ["all-nodes-with-ranges", "num-bigint"]}
# v0.0.3
rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "ed3b4eb72b6e497bbdb4d19dec6621074d724130", default-features = false, features = ["num-bigint"] }
rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "08ebbe40d7776cac6e3ba66277d435056f2b8dca", default-features = false, features = ["num-bigint"] }
# v0.0.3
rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "ed3b4eb72b6e497bbdb4d19dec6621074d724130", default-features = false }
rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "08ebbe40d7776cac6e3ba66277d435056f2b8dca", default-features = false }
# v0.0.3
rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "ed3b4eb72b6e497bbdb4d19dec6621074d724130" , default-features = false, features = ["full-lexer", "all-nodes-with-ranges", "num-bigint"] }
rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "08ebbe40d7776cac6e3ba66277d435056f2b8dca" , default-features = false, features = ["full-lexer", "all-nodes-with-ranges", "num-bigint"] }

[profile.release]
lto = "fat"
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_python_ast/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3680,7 +3680,7 @@ impl AnyNodeRef<'_> {

/// Compares two any node refs by their pointers (referential equality).
pub fn ptr_eq(self, other: AnyNodeRef) -> bool {
self.as_ptr().eq(&other.as_ptr())
self.as_ptr().eq(&other.as_ptr()) && self.kind() == other.kind()
}

/// Returns the node's [`kind`](NodeKind) that has no data associated and is [`Copy`].
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,10 @@ def f2():
# Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513
def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2):
pass


# Regression test for https://github.com/astral-sh/ruff/issues/5176#issuecomment-1598171989
def foo(
b=3 + 2 # comment
):
...
2 changes: 1 addition & 1 deletion crates/ruff_python_formatter/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub fn format_and_debug_print(input: &str, cli: &Cli) -> Result<String> {
}
if cli.print_comments {
println!(
"{:?}",
"{:#?}",
formatted.context().comments().debug(SourceCode::new(input))
);
}
Expand Down
12 changes: 1 addition & 11 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,17 +627,7 @@ fn handle_positional_only_arguments_separator_comment<'a>(
};

let is_last_positional_argument =
// If the preceding node is the identifier for the last positional argument (`a`).
// ```python
// def test(a, /, b): pass
// ```
are_same_optional(last_argument_or_default, arguments.posonlyargs.last().map(|arg| &arg.def))
// If the preceding node is the default for the last positional argument (`10`).
// ```python
// def test(a=10, /, b): pass
// ```
|| are_same_optional(last_argument_or_default, arguments
.posonlyargs.last().and_then(|arg| arg.default.as_deref()));
are_same_optional(last_argument_or_default, arguments.posonlyargs.last());

if !is_last_positional_argument {
return CommentPlacement::Default(comment);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ expression: comments.debug(test_case.source_code)
"trailing": [],
},
Node {
kind: Arg,
range: 90..91,
source: `b`,
kind: ArgWithDefault,
range: 90..94,
source: `b=20`,
}: {
"leading": [
SourceComment {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ expression: comments.debug(test_case.source_code)
"trailing": [],
},
Node {
kind: ExprConstant,
range: 17..19,
source: `10`,
kind: ArgWithDefault,
range: 15..19,
source: `a=10`,
}: {
"leading": [],
"dangling": [],
Expand All @@ -39,9 +39,9 @@ expression: comments.debug(test_case.source_code)
],
},
Node {
kind: Arg,
range: 173..174,
source: `b`,
kind: ArgWithDefault,
range: 173..177,
source: `b=20`,
}: {
"leading": [
SourceComment {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ expression: comments.debug(test_case.source_code)
"trailing": [],
},
Node {
kind: Arg,
kind: ArgWithDefault,
range: 15..16,
source: `a`,
}: {
Expand All @@ -39,7 +39,7 @@ expression: comments.debug(test_case.source_code)
],
},
Node {
kind: Arg,
kind: ArgWithDefault,
range: 166..167,
source: `b`,
}: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ expression: comments.debug(test_case.source_code)
"trailing": [],
},
Node {
kind: Arg,
kind: ArgWithDefault,
range: 15..16,
source: `a`,
}: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ expression: comments.debug(test_case.source_code)
"trailing": [],
},
Node {
kind: Arg,
kind: ArgWithDefault,
range: 15..16,
source: `a`,
}: {
Expand All @@ -39,7 +39,7 @@ expression: comments.debug(test_case.source_code)
],
},
Node {
kind: Arg,
kind: ArgWithDefault,
range: 166..167,
source: `b`,
}: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ expression: comments.debug(test_case.source_code)
---
{
Node {
kind: Arg,
kind: ArgWithDefault,
range: 15..16,
source: `a`,
}: {
Expand Down
7 changes: 7 additions & 0 deletions crates/ruff_python_formatter/src/comments/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,13 @@ impl<'ast> PreorderVisitor<'ast> for CommentsVisitor<'ast> {
self.finish_node(arg);
}

fn visit_arg_with_default(&mut self, arg_with_default: &'ast ArgWithDefault) {
if self.start_node(arg_with_default).is_traverse() {
walk_arg_with_default(self, arg_with_default);
}
self.finish_node(arg_with_default);
}

fn visit_keyword(&mut self, keyword: &'ast Keyword) {
if self.start_node(keyword).is_traverse() {
walk_keyword(self, keyword);
Expand Down
14 changes: 8 additions & 6 deletions crates/ruff_python_formatter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,10 +411,12 @@ Formatted twice:
#[test]
fn quick_test() {
let src = r#"
def test(): ...

# Comment
def with_leading_comment(): ...
def foo(
b=3
+ 2 # comment
):
...
"#;
// Tokenize once
let mut tokens = Vec::new();
Expand All @@ -437,10 +439,10 @@ def with_leading_comment(): ...
// Use `dbg_write!(f, []) instead of `write!(f, [])` in your formatting code to print some IR
// inside of a `Format` implementation
// use ruff_formatter::FormatContext;
// formatted
// dbg!(formatted
// .document()
// .display(formatted.context().source_code());

// .display(formatted.context().source_code()));
//
// dbg!(formatted
// .context()
// .comments()
Expand Down
5 changes: 2 additions & 3 deletions crates/ruff_python_formatter/src/other/arg_with_default.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use rustpython_parser::ast::ArgWithDefault;

use ruff_formatter::write;
use rustpython_parser::ast::ArgWithDefault;

use crate::prelude::*;
use crate::FormatNodeRule;
Expand All @@ -20,7 +19,7 @@ impl FormatNodeRule<ArgWithDefault> for FormatArgWithDefault {

if let Some(default) = default {
let space = def.annotation.is_some().then_some(space());
write!(f, [space, text("="), space, default.format()])?;
write!(f, [space, text("="), space, group(&default.format())])?;
}

Ok(())
Expand Down
33 changes: 9 additions & 24 deletions crates/ruff_python_formatter/src/other/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,19 @@ impl FormatNodeRule<Arguments> for FormatArguments {
let mut last_node: Option<AnyNodeRef> = None;

for arg_with_default in posonlyargs {
joiner.entry(&arg_with_default.into_format());

last_node = Some(
arg_with_default
.default
.as_deref()
.map_or_else(|| (&arg_with_default.def).into(), AnyNodeRef::from),
);
joiner.entry(&arg_with_default.format());

last_node = Some(arg_with_default.into());
}

if !posonlyargs.is_empty() {
joiner.entry(&text("/"));
}

for arg_with_default in args {
joiner.entry(&arg_with_default.into_format());

last_node = Some(
arg_with_default
.default
.as_deref()
.map_or_else(|| (&arg_with_default.def).into(), AnyNodeRef::from),
);
joiner.entry(&arg_with_default.format());

last_node = Some(arg_with_default.into());
}

// kw only args need either a `*args` ahead of them capturing all var args or a `*`
Expand All @@ -74,14 +64,9 @@ impl FormatNodeRule<Arguments> for FormatArguments {
}

for arg_with_default in kwonlyargs {
joiner.entry(&arg_with_default.into_format());

last_node = Some(
arg_with_default
.default
.as_deref()
.map_or_else(|| (&arg_with_default.def).into(), AnyNodeRef::from),
);
joiner.entry(&arg_with_default.format());

last_node = Some(arg_with_default.into());
}

if let Some(kwarg) = kwarg {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ else:
# Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513
def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2):
pass


# Regression test for https://github.com/astral-sh/ruff/issues/5176#issuecomment-1598171989
def foo(
b=3 + 2 # comment
):
...
```


Expand Down Expand Up @@ -223,6 +230,13 @@ else:
# Regression test for https://github.com/python/cpython/blob/7199584ac8632eab57612f595a7162ab8d2ebbc0/Lib/warnings.py#L513
def f(arg1=1, *, kwonlyarg1, kwonlyarg2=2):
pass


# Regression test for https://github.com/astral-sh/ruff/issues/5176#issuecomment-1598171989
def foo(
b=3 + 2, # comment
):
...
```


Loading