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

Format call expressions (without call chaining) #5341

Merged
merged 5 commits into from
Jun 27, 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.

14 changes: 7 additions & 7 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ toml = { version = "0.7.2" }
# v0.0.1
libcst = { git = "https://github.com/charliermarsh/LibCST", rev = "80e4c1399f95e5beb532fdd1e209ad2dbb470438" }

# Please tag the RustPython version everytime you update its revision here.
# Please tag the RustPython version everytime you update its revision here and in fuzz/Cargo.toml
# Tagging the version ensures that older ruff versions continue to build from source even when we rebase our RustPython fork.
# Current tag: v0.0.6
ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8078663b6c914c1cb86993e427764f7c422fc12c" }
rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8078663b6c914c1cb86993e427764f7c422fc12c" , default-features = false, features = ["num-bigint"]}
rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8078663b6c914c1cb86993e427764f7c422fc12c", default-features = false, features = ["num-bigint"] }
rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8078663b6c914c1cb86993e427764f7c422fc12c", default-features = false }
rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "8078663b6c914c1cb86993e427764f7c422fc12c" , default-features = false, features = ["full-lexer", "num-bigint"] }
# Current tag: v0.0.7
ruff_text_size = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "c174bbf1f29527edd43d432326327f16f47ab9e0" }
rustpython-ast = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "c174bbf1f29527edd43d432326327f16f47ab9e0" , default-features = false, features = ["num-bigint"]}
rustpython-format = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "c174bbf1f29527edd43d432326327f16f47ab9e0", default-features = false, features = ["num-bigint"] }
rustpython-literal = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "c174bbf1f29527edd43d432326327f16f47ab9e0", default-features = false }
rustpython-parser = { git = "https://github.com/astral-sh/RustPython-Parser.git", rev = "c174bbf1f29527edd43d432326327f16f47ab9e0" , default-features = false, features = ["full-lexer", "num-bigint"] }

[profile.release]
lto = "fat"
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use ruff_python_ast::source_code::{Indexer, Locator, SourceFileBuilder, Stylist}

use crate::autofix::{fix_file, FixResult};
use crate::directives;
#[cfg(not(fuzzing))]
use crate::jupyter::Notebook;
use crate::linter::{check_path, LinterResult};
use crate::message::{Emitter, EmitterContext, Message, TextEmitter};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
from unittest.mock import MagicMock


def f(*args, **kwargs):
pass

this_is_a_very_long_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd = 1
session = MagicMock()
models = MagicMock()

f()

f(1)

f(x=2)

f(1, x=2)

f(
this_is_a_very_long_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd
)
f(
this_is_a_very_long_keyword_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd=1
)

f(
1,
mixed_very_long_arguments=1,
)

f(
this_is_a_very_long_argument_asökdhflakjslaslhfdlaffahflahsöfdhasägporejfäalkdsjäfalisjäfdlkasjd,
these_arguments_have_values_that_need_to_break_because_they_are_too_long1=(100000 - 100000000000),
these_arguments_have_values_that_need_to_break_because_they_are_too_long2="akshfdlakjsdfad" + "asdfasdfa",
these_arguments_have_values_that_need_to_break_because_they_are_too_long3=session,
)

f(
# dangling comment
)


f(
only=1, short=1, arguments=1
)

f(
hey_this_is_a_long_call, it_has_funny_attributes_that_breaks_into_three_lines=1
)

f(
hey_this_is_a_very_long_call=1, it_has_funny_attributes_asdf_asdf=1, too_long_for_the_line=1, really=True
)

# TODO(konstin): Call chains/fluent interface (https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#call-chains)
result = (
session.query(models.Customer.id)
.filter(
models.Customer.account_id == 10000,
models.Customer.email == "user@example.org",
)
.order_by(models.Customer.id.asc())
.all()
)
# TODO(konstin): Black has this special case for comment placement where everything stays in one line
f(
"aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa", "aaaaaaaa"
)

f(
session,
b=1,
** # oddly placed end-of-line comment
dict()
)
f(
session,
b=1,
**
# oddly placed own line comment
dict()
)

52 changes: 29 additions & 23 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,7 @@ fn handle_dict_unpacking_comment<'a>(
match comment.enclosing_node() {
// TODO: can maybe also add AnyNodeRef::Arguments here, but tricky to test due to
// https://github.com/astral-sh/ruff/issues/5176
AnyNodeRef::ExprDict(_) => {}
AnyNodeRef::ExprDict(_) | AnyNodeRef::Keyword(_) => {}
_ => {
return CommentPlacement::Default(comment);
}
Expand All @@ -1015,12 +1015,22 @@ fn handle_dict_unpacking_comment<'a>(
)
.skip_trivia();

// if the remaining tokens from the previous node are exactly `**`,
// re-assign the comment to the one that follows the stars
let mut count = 0;

// we start from the preceding node but we skip its token
for token in tokens.by_ref() {
// Skip closing parentheses that are not part of the node range
if token.kind == TokenKind::RParen {
continue;
}
// The Keyword case
if token.kind == TokenKind::Star {
count += 1;
break;
}
// The dict case
debug_assert!(
matches!(
token,
Expand All @@ -1034,9 +1044,6 @@ fn handle_dict_unpacking_comment<'a>(
break;
}

// if the remaining tokens from the previous node is exactly `**`,
// re-assign the comment to the one that follows the stars
let mut count = 0;
for token in tokens {
if token.kind != TokenKind::Star {
return CommentPlacement::Default(comment);
Expand All @@ -1050,19 +1057,19 @@ fn handle_dict_unpacking_comment<'a>(
CommentPlacement::Default(comment)
}

// Own line comments coming after the node are always dangling comments
// ```python
// (
// a
// # trailing a comment
// . # dangling comment
// # or this
// b
// )
// ```
/// Own line comments coming after the node are always dangling comments
/// ```python
/// (
/// a
/// # trailing a comment
/// . # dangling comment
/// # or this
/// b
/// )
/// ```
fn handle_attribute_comment<'a>(
comment: DecoratedComment<'a>,
locator: &Locator,
_locator: &Locator,
) -> CommentPlacement<'a> {
let Some(attribute) = comment.enclosing_node().expr_attribute() else {
return CommentPlacement::Default(comment);
Expand All @@ -1073,14 +1080,13 @@ fn handle_attribute_comment<'a>(
return CommentPlacement::Default(comment);
}

let between_value_and_attr = TextRange::new(attribute.value.end(), attribute.attr.start());

let dot = SimpleTokenizer::new(locator.contents(), between_value_and_attr)
.skip_trivia()
.next()
.expect("Expected the `.` character after the value");

if TextRange::new(dot.end(), attribute.attr.start()).contains(comment.slice().start()) {
if TextRange::new(attribute.value.end(), attribute.attr.start())
.contains(comment.slice().start())
{
// ```text
// value . attr
// ^^^^^^^ the range of dangling comments
Comment on lines +1087 to +1088
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

// ```
if comment.line_position().is_end_of_line() {
// Attach to node with b
// ```python
Expand Down
84 changes: 71 additions & 13 deletions crates/ruff_python_formatter/src/expression/expr_call.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use crate::comments::Comments;
use crate::builders::PyFormatterExtensions;
use crate::comments::{dangling_comments, Comments};
use crate::expression::parentheses::{
default_expression_needs_parentheses, NeedsParentheses, Parentheses, Parenthesize,
};
use crate::{not_yet_implemented_custom_text, FormatNodeRule, PyFormatter};
use crate::{AsFormat, FormatNodeRule, PyFormatter};
use ruff_formatter::prelude::{format_with, group, soft_block_indent, text};
use ruff_formatter::{write, Buffer, FormatResult};
use rustpython_parser::ast::ExprCall;

Expand All @@ -11,19 +13,75 @@ pub struct FormatExprCall;

impl FormatNodeRule<ExprCall> for FormatExprCall {
fn fmt_fields(&self, item: &ExprCall, f: &mut PyFormatter) -> FormatResult<()> {
if item.args.is_empty() && item.keywords.is_empty() {
write!(
f,
[not_yet_implemented_custom_text("NOT_IMPLEMENTED_call()")]
)
} else {
write!(
let ExprCall {
range: _,
func,
args,
keywords,
} = item;

// We have a case with `f()` without any argument, which is a special case because we can
// have a comment with no node attachment inside:
// ```python
// f(
// # This function has a dangling comment
// )
// ```
if args.is_empty() && keywords.is_empty() {
let comments = f.context().comments().clone();
let comments = comments.dangling_comments(item);
return write!(
f,
[not_yet_implemented_custom_text(
"NOT_IMPLEMENTED_call(NOT_IMPLEMENTED_arg)"
)]
)
[
func.format(),
text("("),
dangling_comments(comments),
text(")")
]
);
}

let all_args = format_with(|f| {
f.join_comma_separated()
.entries(
// We have the parentheses from the call so the arguments never need any
args.iter()
.map(|arg| (arg, arg.format().with_options(Parenthesize::Never))),
)
.nodes(keywords.iter())
.finish()
});

write!(
f,
[
func.format(),
text("("),
// The outer group is for things like
// ```python
// get_collection(
// hey_this_is_a_very_long_call,
// it_has_funny_attributes_asdf_asdf,
// too_long_for_the_line,
// really=True,
// )
// ```
// The inner group is for things like:
// ```python
// get_collection(
// hey_this_is_a_very_long_call, it_has_funny_attributes_asdf_asdf, really=True
// )
// ```
// TODO(konstin): Doesn't work see wrongly formatted test
&group(&soft_block_indent(&group(&all_args))),
text(")")
]
)
}

fn fmt_dangling_comments(&self, _node: &ExprCall, _f: &mut PyFormatter) -> FormatResult<()> {
// Handled in `fmt_fields`
Ok(())
}
}

Expand Down
9 changes: 5 additions & 4 deletions crates/ruff_python_formatter/src/other/keyword.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ impl FormatNodeRule<Keyword> for FormatKeyword {
arg,
value,
} = item;
if let Some(argument) = arg {
write!(f, [argument.format(), text("=")])?;
if let Some(arg) = arg {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I prefer to move the logic that is common for all branches out of the if. I find that this improves readability. I otherwise have to manually parse both branches to understand what's the same/different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i want to keep those two specifically since they are one entry in a list each and because each branch has its own comment handling

write!(f, [arg.format(), text("="), value.format()])
} else {
// Comments after the stars are reassigned as trailing value comments
write!(f, [text("**"), value.format()])
}

value.format().fmt(f)
}
}
Loading