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

Improve line length handling of end-of-line breakables in call chains #463

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open
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
10 changes: 10 additions & 0 deletions fixtures/small/block_param_line_length_actual.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
ThisIsAReallyLongClassName::WithSomeModulesInsideItThatHaveAMethodThatWeWillCallRightAroundHereeeee.do_stuff(boom) do |foo|
some_stuff!
end

ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls.foo.bar.baz.quux.what_comes_after_quux_idk_aaaahhhh.map { |model|
body_of_the_call
}

ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls.foo.bar.baz.quux.what_comes_after_quux_idk_aaaahhhhhhh.map(&:foo)
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls::ThisIsAReallyLongClassName::ButSlightShorter.new(foo: bar_baz_quuz)
25 changes: 25 additions & 0 deletions fixtures/small/block_param_line_length_expected.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
ThisIsAReallyLongClassName::WithSomeModulesInsideItThatHaveAMethodThatWeWillCallRightAroundHereeeee
.do_stuff(boom) do |foo|
some_stuff!
end

ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls
.foo
.bar
.baz
.quux
.what_comes_after_quux_idk_aaaahhhh
.map { |model|
body_of_the_call
}

ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls
.foo
.bar
.baz
.quux
.what_comes_after_quux_idk_aaaahhhhhhh
.map(&:foo)
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls::ThisIsAReallyLongClassName::ButSlightShorter.new(
foo: bar_baz_quuz
)
25 changes: 13 additions & 12 deletions fixtures/small/long_blockvar_expected.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
things.each do |
omg:,
really:,
dang:,
long:,
blockvar:,
that_is_so_long_if_you_write_this:,
you_should_refactor:,
like_really_this_is_so_long:
|
do_things!
end
things
.each do |
omg:,
really:,
dang:,
long:,
blockvar:,
that_is_so_long_if_you_write_this:,
you_should_refactor:,
like_really_this_is_so_long:
|
do_things!
end
5 changes: 5 additions & 0 deletions fixtures/small/multiline_chain_in_block_actual.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,8 @@
def ajax_get(route)
super
end

class Foo
sig {override.returns(T::Array[T.class_of(Some::Really::Long::Name::ThatshouldprobablybealisedbutisntbecauseThis::IsATestStub)])}
def example = begin; end
end
10 changes: 10 additions & 0 deletions fixtures/small/multiline_chain_in_block_expected.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,13 @@
def ajax_get(route)
super
end

class Foo
sig {
override.returns(
T::Array[T.class_of(Some::Really::Long::Name::ThatshouldprobablybealisedbutisntbecauseThis::IsATestStub)]
)
}
def example = begin
end
end
4 changes: 2 additions & 2 deletions librubyfmt/src/delimiters.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::line_tokens::ConcreteLineToken;

#[derive(Debug, Clone)]
#[derive(Debug, Clone, Eq, PartialEq)]
struct DelimiterPair {
open: String,
close: String,
Expand All @@ -12,7 +12,7 @@ impl DelimiterPair {
}
}

#[derive(Debug, Clone)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct BreakableDelims {
single_line: DelimiterPair,
multi_line: DelimiterPair,
Expand Down
72 changes: 58 additions & 14 deletions librubyfmt/src/render_targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,38 +252,82 @@ impl AbstractTokenTarget for BreakableCallChainEntry {
// individual line and get _that_ max length.
let mut tokens = self.tokens.clone();
if tokens.len() > 2 {
if let Some(AbstractLineToken::ConcreteLineToken(ConcreteLineToken::End)) =
tokens.get(tokens.len() - 2)
{
// Pop off all tokens that make up the `do`/`end` block (but not `do`!),
let index = tokens.len() - 2;
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 wholly recognize that this method is getting to be quite convoluted (it was already a bit of a mess before, and this PR only adds to it), but call chains have a sufficient enough number of ~weird cases to warrant the complexity, and it's well tested enough that I feel like this is okay to leave for now. I'm not 100% sure the best way we'd clean it up without making some concessions on the final output anyways.

let token = tokens.get_mut(index).unwrap();
if matches!(
token,
AbstractLineToken::ConcreteLineToken(ConcreteLineToken::End)
) {
// Pop off all tokens that make up the block (but not the block params!),
// since we assume that the block contents will handle their own line
// length appropriately.
while let Some(token) = tokens.last() {
if matches!(
token,
AbstractLineToken::ConcreteLineToken(ConcreteLineToken::DoKeyword)
AbstractLineToken::BreakableEntry(BreakableEntry { delims, .. }) if *delims == BreakableDelims::for_block_params()
) {
break;
}
tokens.pop();
}
} else if let AbstractLineToken::BreakableEntry(BreakableEntry {
delims,
ref mut tokens,
..
}) = token
{
if *delims == BreakableDelims::for_brace_block() {
if let Some(AbstractLineToken::BreakableEntry(BreakableEntry {
delims, ..
})) = tokens.first()
{
if *delims == BreakableDelims::for_block_params() {
// Wipe away the body of the block and leave only the params
*tokens = vec![tokens.first().unwrap().clone()];
} else {
// No params, so wipe away the whole thing
*tokens = Vec::new();
}
} else {
// No params, so wipe away the whole thing
*tokens = Vec::new();
}
}
}
}

if let Some(AbstractLineToken::BreakableEntry(_)) = tokens.first() {
tokens.remove(0);
}
// EndCallChainIndent, which we don't care about
tokens.pop();
// If the last breakable extends beyond the line length but the call chain doesn't,
// the breakable will break itself, e.g.
// ```ruby
// # ↓ if the break is here, we'll break the parens instead of the call chain
// AssumeThisIs.one_hundred_twenty_characters(breaks_here)
// ```
if let Some(AbstractLineToken::BreakableEntry(_)) = tokens.last() {
if let Some(AbstractLineToken::ConcreteLineToken(ConcreteLineToken::EndCallChainIndent)) =
tokens.last()
{
tokens.pop();
}
let call_count = tokens
.iter()
.filter(|t| {
matches!(
t,
AbstractLineToken::ConcreteLineToken(
ConcreteLineToken::Dot | ConcreteLineToken::LonelyOperator
)
)
})
.count();
// If the last breakable is multiline (and not a block/block params), ignore it. The user likely
// intentionally chose a line break strategy, so try our best to respect it.
//
// However, if there's only one item in the chain, try our best to leave that in place.
// `foo\n.bar` is always a little awkward.
if let Some(AbstractLineToken::BreakableEntry(be)) = tokens.last() {
if (call_count == 1 || be.is_multiline())
&& be.delims != BreakableDelims::for_brace_block()
&& be.delims != BreakableDelims::for_block_params()
{
tokens.pop();
}
}
tokens.insert(
0,
AbstractLineToken::ConcreteLineToken(
Expand Down
Loading