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

Properly multiline user-multilined call chains #382

Merged
merged 3 commits into from
Dec 3, 2022
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
5 changes: 4 additions & 1 deletion fixtures/large/rspec_mocks_proxy_expected.rb
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,10 @@ def reset
class PartialDoubleProxy < Proxy
def original_method_handle_for(message)
if any_instance_class_recorder_observing_method?(@object.class, message)
message = ::RSpec::Mocks.space.any_instance_recorder_for(@object.class).build_alias_method_name(message)
message = ::RSpec::Mocks
.space
.any_instance_recorder_for(@object.class)
.build_alias_method_name(message)
end

::RSpec::Support.method_handle_for(@object, message)
Expand Down
7 changes: 5 additions & 2 deletions fixtures/small/comments_at_indentation_changes_expected.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
it "" do
# Should only download the patches twice
Test::Mock.expects(Util, :download_file).twice.returns(true)
Test::Mock
.expects(Util, :download_file)
# Should only download the patches twice
.twice
.returns(true)
end

# Make some fake thing
Expand Down
4 changes: 3 additions & 1 deletion fixtures/small/method_annotation_expected.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ def empty_example
end

sig do
params(a: T::Array[String], b: T::Hash[Symbol, String]).returns(T::Set[Symbol]).checked(:tests)
params(a: T::Array[String], b: T::Hash[Symbol, String])
.returns(T::Set[Symbol])
.checked(:tests)
end
def do_the_thing(a, b)
puts(a)
Expand Down
22 changes: 22 additions & 0 deletions fixtures/small/method_chains_actual.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,28 @@
b: ""
)

foo.bar.baz

foo.bar
.baz

# If they're all on the same line but different from
# the first receiver, consider that "on one line"
foo
.bar.baz

foo::bar
&.nil?

foo::bar
&.nil?::klass
.true?

Class
&.new
.call!


def example
things
.map do |thing|
Expand Down
20 changes: 20 additions & 0 deletions fixtures/small/method_chains_expected.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,26 @@
b: ""
)

foo.bar.baz

foo
.bar
.baz

# If they're all on the same line but different from
# the first receiver, consider that "on one line"
foo.bar.baz

foo::bar&.nil?

foo::bar
&.nil?::klass
.true?
Comment on lines +30 to +32
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is blursed but technically legal -- I handled it by just multilining where possible, but open to other thoughts on how we should handle :: calls.

Copy link
Owner

Choose a reason for hiding this comment

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

I hate you

Copy link
Owner

Choose a reason for hiding this comment

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

(j/k obvi)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think pretty much every fixture I write should just have # im so sorry at the top of them


Class
&.new
.call!

def example
things
.map do |thing|
Expand Down
4 changes: 4 additions & 0 deletions librubyfmt/rubyfmt_lib.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@ def escape_percent_array_paren_content(part, pattern)
end
end

def on_op(*_args)
super + [[lineno, lineno]]
end

def on_next(*_args)
super + [start_end_for_keyword('next')]
end
Expand Down
50 changes: 46 additions & 4 deletions librubyfmt/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2789,7 +2789,16 @@ fn format_call_chain_elements(
ps.start_indent();
has_indented = true;
}
if render_multiline_chain {
let is_double_colon = match &d {
DotTypeOrOp::ColonColon(_) => true,
DotTypeOrOp::StringDot(val) => val == "::",
_ => false,
};
if render_multiline_chain
// Separating `::` calls with a newline
// isn't valid syntax
&& !is_double_colon
{
ps.emit_newline();
ps.emit_indent();
}
Expand Down Expand Up @@ -2844,9 +2853,10 @@ fn is_heredoc_call_chain_with_breakables(cc_elements: &[CallChainElement]) -> bo
///
/// ## High-level rules
///
/// The two main rules that govern whether or not to multiline a method chain is to split across multiple lines if
/// (1) the whole chain exceeds the maximum line length or
/// (2) the chain contains blocks that are split across multiple lines
/// The three main rules that govern whether or not to multiline a method chain is to split across multiple lines if
/// (1) the user multilined the chain,
/// (2) the whole chain exceeds the maximum line length, or
/// (3) the chain contains blocks that are split across multiple lines
///
/// That said, both of these have some *very large* asterisks, since there are a lot of contexts in which these
/// have special cases for various reasons (see below).
Expand Down Expand Up @@ -2884,6 +2894,38 @@ fn should_multiline_call_chain(ps: &mut dyn ConcreteParserState, method_call: &M
CallChainElement::ArgsAddStarOrExpressionListOrArgsForward(args, start_end),
]);

let all_op_locations = call_chain_to_check
.iter()
.filter_map(|cc_elem| match cc_elem {
CallChainElement::DotTypeOrOp(dot_type_or_op) => {
match dot_type_or_op {
// ColonColon is specially represented in the parser, and
// it can't be properly multilined anyways, so we ignore it here
DotTypeOrOp::ColonColon(..) => None,
DotTypeOrOp::StringDot(..) => None,
DotTypeOrOp::Op(Op(.., start_end))
| DotTypeOrOp::DotType(
DotType::LonelyOperator(LonelyOperator(_, start_end))
| DotType::Dot(Dot(_, start_end)),
) => Some(start_end.clone()),
DotTypeOrOp::Period(Period(.., linecol)) => {
Some(StartEnd(linecol.0, linecol.0))
}
}
}
_ => None,
})
.collect::<Vec<StartEnd>>();
// Multiline the chain if all the operators (dots, double colons, etc.) are not on the same line
if let Some(first_op_start_end) = all_op_locations.first() {
let chain_is_user_multilined = !all_op_locations
.iter()
.all(|op_start_end| op_start_end == first_op_start_end);
if chain_is_user_multilined {
return true;
}
}

// Ignore chains that are basically only method calls, e.g.
// ````ruby
// Thing.foo(args)
Expand Down
8 changes: 4 additions & 4 deletions librubyfmt/src/ripper_tree_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ pub struct BlockArg(pub blockarg_tag, pub Ident);
#[derive(Deserialize, Debug, Clone)]
pub struct LineCol(pub LineNumber, pub u64);

#[derive(Deserialize, Debug, Clone)]
#[derive(Deserialize, Debug, Clone, Eq, PartialEq)]
pub struct StartEnd(pub LineNumber, pub LineNumber);

impl StartEnd {
Expand Down Expand Up @@ -1527,19 +1527,19 @@ pub struct Equals(pub equals_tag);

def_tag!(dot_tag, ".");
#[derive(Deserialize, Debug, Clone)]
pub struct Dot(pub dot_tag);
pub struct Dot(pub dot_tag, pub StartEnd);

def_tag!(colon_colon_tag, "::");
#[derive(Deserialize, Debug, Clone)]
pub struct ColonColon(pub colon_colon_tag);

def_tag!(lonely_operator_tag, "&.");
#[derive(Deserialize, Debug, Clone)]
pub struct LonelyOperator(pub lonely_operator_tag);
pub struct LonelyOperator(pub lonely_operator_tag, pub StartEnd);

def_tag!(op_tag, "@op");
#[derive(Deserialize, Debug, Clone)]
pub struct Op(pub op_tag, pub Operator, pub LineCol);
pub struct Op(pub op_tag, pub Operator, pub LineCol, pub StartEnd);

#[derive(RipperDeserialize, Debug, Clone)]
pub enum Operator {
Expand Down