Skip to content

Commit 7a4626f

Browse files
authored
Improve line length handling of end-of-line breakables in call chains (#463)
* Fixtures * Keep block params intact when checking line length of call chains * fmt * Also apply to params at the end of the line * Try and make single-call breakables less ocassionally-gross * fmt
1 parent bc3b585 commit 7a4626f

File tree

7 files changed

+123
-28
lines changed

7 files changed

+123
-28
lines changed
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
ThisIsAReallyLongClassName::WithSomeModulesInsideItThatHaveAMethodThatWeWillCallRightAroundHereeeee.do_stuff(boom) do |foo|
2+
some_stuff!
3+
end
4+
5+
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls.foo.bar.baz.quux.what_comes_after_quux_idk_aaaahhhh.map { |model|
6+
body_of_the_call
7+
}
8+
9+
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls.foo.bar.baz.quux.what_comes_after_quux_idk_aaaahhhhhhh.map(&:foo)
10+
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls::ThisIsAReallyLongClassName::ButSlightShorter.new(foo: bar_baz_quuz)
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
ThisIsAReallyLongClassName::WithSomeModulesInsideItThatHaveAMethodThatWeWillCallRightAroundHereeeee
2+
.do_stuff(boom) do |foo|
3+
some_stuff!
4+
end
5+
6+
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls
7+
.foo
8+
.bar
9+
.baz
10+
.quux
11+
.what_comes_after_quux_idk_aaaahhhh
12+
.map { |model|
13+
body_of_the_call
14+
}
15+
16+
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls
17+
.foo
18+
.bar
19+
.baz
20+
.quux
21+
.what_comes_after_quux_idk_aaaahhhhhhh
22+
.map(&:foo)
23+
ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls::ThisIsAReallyLongClassName::ButSlightShorter.new(
24+
foo: bar_baz_quuz
25+
)
Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
1-
things.each do |
2-
omg:,
3-
really:,
4-
dang:,
5-
long:,
6-
blockvar:,
7-
that_is_so_long_if_you_write_this:,
8-
you_should_refactor:,
9-
like_really_this_is_so_long:
10-
|
11-
do_things!
12-
end
1+
things
2+
.each do |
3+
omg:,
4+
really:,
5+
dang:,
6+
long:,
7+
blockvar:,
8+
that_is_so_long_if_you_write_this:,
9+
you_should_refactor:,
10+
like_really_this_is_so_long:
11+
|
12+
do_things!
13+
end

fixtures/small/multiline_chain_in_block_actual.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,8 @@
66
def ajax_get(route)
77
super
88
end
9+
10+
class Foo
11+
sig {override.returns(T::Array[T.class_of(Some::Really::Long::Name::ThatshouldprobablybealisedbutisntbecauseThis::IsATestStub)])}
12+
def example = begin; end
13+
end

fixtures/small/multiline_chain_in_block_expected.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,13 @@
77
def ajax_get(route)
88
super
99
end
10+
11+
class Foo
12+
sig {
13+
override.returns(
14+
T::Array[T.class_of(Some::Really::Long::Name::ThatshouldprobablybealisedbutisntbecauseThis::IsATestStub)]
15+
)
16+
}
17+
def example = begin
18+
end
19+
end

librubyfmt/src/delimiters.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use crate::line_tokens::ConcreteLineToken;
22

3-
#[derive(Debug, Clone)]
3+
#[derive(Debug, Clone, Eq, PartialEq)]
44
struct DelimiterPair {
55
open: String,
66
close: String,
@@ -12,7 +12,7 @@ impl DelimiterPair {
1212
}
1313
}
1414

15-
#[derive(Debug, Clone)]
15+
#[derive(Debug, Clone, PartialEq, Eq)]
1616
pub struct BreakableDelims {
1717
single_line: DelimiterPair,
1818
multi_line: DelimiterPair,

librubyfmt/src/render_targets.rs

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -252,38 +252,82 @@ impl AbstractTokenTarget for BreakableCallChainEntry {
252252
// individual line and get _that_ max length.
253253
let mut tokens = self.tokens.clone();
254254
if tokens.len() > 2 {
255-
if let Some(AbstractLineToken::ConcreteLineToken(ConcreteLineToken::End)) =
256-
tokens.get(tokens.len() - 2)
257-
{
258-
// Pop off all tokens that make up the `do`/`end` block (but not `do`!),
255+
let index = tokens.len() - 2;
256+
let token = tokens.get_mut(index).unwrap();
257+
if matches!(
258+
token,
259+
AbstractLineToken::ConcreteLineToken(ConcreteLineToken::End)
260+
) {
261+
// Pop off all tokens that make up the block (but not the block params!),
259262
// since we assume that the block contents will handle their own line
260263
// length appropriately.
261264
while let Some(token) = tokens.last() {
262265
if matches!(
263266
token,
264-
AbstractLineToken::ConcreteLineToken(ConcreteLineToken::DoKeyword)
267+
AbstractLineToken::BreakableEntry(BreakableEntry { delims, .. }) if *delims == BreakableDelims::for_block_params()
265268
) {
266269
break;
267270
}
268271
tokens.pop();
269272
}
273+
} else if let AbstractLineToken::BreakableEntry(BreakableEntry {
274+
delims,
275+
ref mut tokens,
276+
..
277+
}) = token
278+
{
279+
if *delims == BreakableDelims::for_brace_block() {
280+
if let Some(AbstractLineToken::BreakableEntry(BreakableEntry {
281+
delims, ..
282+
})) = tokens.first()
283+
{
284+
if *delims == BreakableDelims::for_block_params() {
285+
// Wipe away the body of the block and leave only the params
286+
*tokens = vec![tokens.first().unwrap().clone()];
287+
} else {
288+
// No params, so wipe away the whole thing
289+
*tokens = Vec::new();
290+
}
291+
} else {
292+
// No params, so wipe away the whole thing
293+
*tokens = Vec::new();
294+
}
295+
}
270296
}
271297
}
272298

273299
if let Some(AbstractLineToken::BreakableEntry(_)) = tokens.first() {
274300
tokens.remove(0);
275301
}
276-
// EndCallChainIndent, which we don't care about
277-
tokens.pop();
278-
// If the last breakable extends beyond the line length but the call chain doesn't,
279-
// the breakable will break itself, e.g.
280-
// ```ruby
281-
// # ↓ if the break is here, we'll break the parens instead of the call chain
282-
// AssumeThisIs.one_hundred_twenty_characters(breaks_here)
283-
// ```
284-
if let Some(AbstractLineToken::BreakableEntry(_)) = tokens.last() {
302+
if let Some(AbstractLineToken::ConcreteLineToken(ConcreteLineToken::EndCallChainIndent)) =
303+
tokens.last()
304+
{
285305
tokens.pop();
286306
}
307+
let call_count = tokens
308+
.iter()
309+
.filter(|t| {
310+
matches!(
311+
t,
312+
AbstractLineToken::ConcreteLineToken(
313+
ConcreteLineToken::Dot | ConcreteLineToken::LonelyOperator
314+
)
315+
)
316+
})
317+
.count();
318+
// If the last breakable is multiline (and not a block/block params), ignore it. The user likely
319+
// intentionally chose a line break strategy, so try our best to respect it.
320+
//
321+
// However, if there's only one item in the chain, try our best to leave that in place.
322+
// `foo\n.bar` is always a little awkward.
323+
if let Some(AbstractLineToken::BreakableEntry(be)) = tokens.last() {
324+
if (call_count == 1 || be.is_multiline())
325+
&& be.delims != BreakableDelims::for_brace_block()
326+
&& be.delims != BreakableDelims::for_block_params()
327+
{
328+
tokens.pop();
329+
}
330+
}
287331
tokens.insert(
288332
0,
289333
AbstractLineToken::ConcreteLineToken(

0 commit comments

Comments
 (0)