From ada3c39c75157be264329ed1ccf8b2cff533c6f5 Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Wed, 21 Feb 2024 05:55:21 +0000 Subject: [PATCH 1/6] Fixtures --- fixtures/small/block_param_line_length_actual.rb | 7 +++++++ fixtures/small/block_param_line_length_expected.rb | 14 ++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 fixtures/small/block_param_line_length_actual.rb create mode 100644 fixtures/small/block_param_line_length_expected.rb diff --git a/fixtures/small/block_param_line_length_actual.rb b/fixtures/small/block_param_line_length_actual.rb new file mode 100644 index 00000000..eea45f93 --- /dev/null +++ b/fixtures/small/block_param_line_length_actual.rb @@ -0,0 +1,7 @@ +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 +} \ No newline at end of file diff --git a/fixtures/small/block_param_line_length_expected.rb b/fixtures/small/block_param_line_length_expected.rb new file mode 100644 index 00000000..52b1a148 --- /dev/null +++ b/fixtures/small/block_param_line_length_expected.rb @@ -0,0 +1,14 @@ +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 + } From 378b98aca36889f84960699833aa5101431813d1 Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Wed, 21 Feb 2024 05:55:55 +0000 Subject: [PATCH 2/6] Keep block params intact when checking line length of call chains --- librubyfmt/src/delimiters.rs | 4 ++-- librubyfmt/src/render_targets.rs | 32 +++++++++++++++++++++++--------- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/librubyfmt/src/delimiters.rs b/librubyfmt/src/delimiters.rs index 08317dc4..50959713 100644 --- a/librubyfmt/src/delimiters.rs +++ b/librubyfmt/src/delimiters.rs @@ -1,6 +1,6 @@ use crate::line_tokens::ConcreteLineToken; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Eq, PartialEq)] struct DelimiterPair { open: String, close: String, @@ -12,7 +12,7 @@ impl DelimiterPair { } } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct BreakableDelims { single_line: DelimiterPair, multi_line: DelimiterPair, diff --git a/librubyfmt/src/render_targets.rs b/librubyfmt/src/render_targets.rs index f17e71c0..acae4ab0 100644 --- a/librubyfmt/src/render_targets.rs +++ b/librubyfmt/src/render_targets.rs @@ -252,37 +252,51 @@ 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; + 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()]; + } + } + } } } if let Some(AbstractLineToken::BreakableEntry(_)) = tokens.first() { tokens.remove(0); } - // EndCallChainIndent, which we don't care about - tokens.pop(); + if let Some(AbstractLineToken::ConcreteLineToken(ConcreteLineToken::EndCallChainIndent)) = tokens.last() { + 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() { - tokens.pop(); + if let Some(AbstractLineToken::BreakableEntry(be)) = tokens.last() { + // For block params, always pop it if it's multiline, otherwise we'd *always* multiline the whole block regardless of the contents. + // Never pop brace blocks, since we've already cleared their contents above, so now we're only looking at the params, which are still relevant. + if (be.delims != BreakableDelims::for_block_params() || be.is_multiline()) && be.delims != BreakableDelims::for_brace_block() { + tokens.pop(); + } } tokens.insert( 0, From b9827fb6ad086a4c2b6536247fe6c22325ac4925 Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Wed, 21 Feb 2024 05:58:54 +0000 Subject: [PATCH 3/6] fmt --- librubyfmt/src/render_targets.rs | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/librubyfmt/src/render_targets.rs b/librubyfmt/src/render_targets.rs index acae4ab0..c110103e 100644 --- a/librubyfmt/src/render_targets.rs +++ b/librubyfmt/src/render_targets.rs @@ -254,7 +254,10 @@ impl AbstractTokenTarget for BreakableCallChainEntry { if tokens.len() > 2 { let index = tokens.len() - 2; let token = tokens.get_mut(index).unwrap(); - if matches!(token, AbstractLineToken::ConcreteLineToken(ConcreteLineToken::End)) { + 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. @@ -267,12 +270,20 @@ impl AbstractTokenTarget for BreakableCallChainEntry { } tokens.pop(); } - } else if let AbstractLineToken::BreakableEntry(BreakableEntry { delims, ref mut tokens, .. }) = token { + } 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 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()]; + *tokens = vec![tokens.first().unwrap().clone()]; } } } @@ -282,7 +293,9 @@ impl AbstractTokenTarget for BreakableCallChainEntry { if let Some(AbstractLineToken::BreakableEntry(_)) = tokens.first() { tokens.remove(0); } - if let Some(AbstractLineToken::ConcreteLineToken(ConcreteLineToken::EndCallChainIndent)) = tokens.last() { + if let Some(AbstractLineToken::ConcreteLineToken(ConcreteLineToken::EndCallChainIndent)) = + tokens.last() + { tokens.pop(); } // If the last breakable extends beyond the line length but the call chain doesn't, @@ -294,7 +307,9 @@ impl AbstractTokenTarget for BreakableCallChainEntry { if let Some(AbstractLineToken::BreakableEntry(be)) = tokens.last() { // For block params, always pop it if it's multiline, otherwise we'd *always* multiline the whole block regardless of the contents. // Never pop brace blocks, since we've already cleared their contents above, so now we're only looking at the params, which are still relevant. - if (be.delims != BreakableDelims::for_block_params() || be.is_multiline()) && be.delims != BreakableDelims::for_brace_block() { + if (be.delims != BreakableDelims::for_block_params() || be.is_multiline()) + && be.delims != BreakableDelims::for_brace_block() + { tokens.pop(); } } From 01987e33d4a3ae9b080659406ec29140172a68cf Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Wed, 21 Feb 2024 18:03:16 +0000 Subject: [PATCH 4/6] Also apply to params at the end of the line --- .../small/block_param_line_length_actual.rb | 5 +- .../small/block_param_line_length_expected.rb | 10 +++ .../breakables_over_line_length_expected.rb | 6 +- .../small/heredoc_method_call_expected.rb | 61 ++++++++++--------- librubyfmt/src/render_targets.rs | 20 +++--- 5 files changed, 56 insertions(+), 46 deletions(-) diff --git a/fixtures/small/block_param_line_length_actual.rb b/fixtures/small/block_param_line_length_actual.rb index eea45f93..4f87e0fa 100644 --- a/fixtures/small/block_param_line_length_actual.rb +++ b/fixtures/small/block_param_line_length_actual.rb @@ -4,4 +4,7 @@ ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls.foo.bar.baz.quux.what_comes_after_quux_idk_aaaahhhh.map { |model| body_of_the_call -} \ No newline at end of file +} + +ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls.foo.bar.baz.quux.what_comes_after_quux_idk_aaaahhhhhhh.map(&:foo) +ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls::ThisIsAReallyLongClassName::ButSlightShorter.new(foo: bar_baz_quuz) \ No newline at end of file diff --git a/fixtures/small/block_param_line_length_expected.rb b/fixtures/small/block_param_line_length_expected.rb index 52b1a148..535a5a6e 100644 --- a/fixtures/small/block_param_line_length_expected.rb +++ b/fixtures/small/block_param_line_length_expected.rb @@ -12,3 +12,13 @@ .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) diff --git a/fixtures/small/breakables_over_line_length_expected.rb b/fixtures/small/breakables_over_line_length_expected.rb index 6d4da475..79b4d17a 100644 --- a/fixtures/small/breakables_over_line_length_expected.rb +++ b/fixtures/small/breakables_over_line_length_expected.rb @@ -18,9 +18,7 @@ ReallyLongThing ] -if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample.calls_a_thing( - a, - b - ) +if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample + .calls_a_thing(a, b) puts("") end diff --git a/fixtures/small/heredoc_method_call_expected.rb b/fixtures/small/heredoc_method_call_expected.rb index 903e4ff9..324347ea 100644 --- a/fixtures/small/heredoc_method_call_expected.rb +++ b/fixtures/small/heredoc_method_call_expected.rb @@ -1,34 +1,35 @@ class William::Carlos::Williams - landscape_with_the_fall_of_icarus = T.let( - new( - <<~LANDSCAPE - According to Brueghel - when Icarus fell - it was spring - - a farmer was ploughing - his field - the whole pageantry - - of the year was - awake tingling - with itself - - sweating in the sun - that melted - the wings' wax - - unsignificantly - off the coast - there was - - a splash quite unnoticed - this was - Icarus drowning - LANDSCAPE - ), - Williams - ) + landscape_with_the_fall_of_icarus = T + .let( + new( + <<~LANDSCAPE + According to Brueghel + when Icarus fell + it was spring + + a farmer was ploughing + his field + the whole pageantry + + of the year was + awake tingling + with itself + + sweating in the sun + that melted + the wings' wax + + unsignificantly + off the coast + there was + + a splash quite unnoticed + this was + Icarus drowning + LANDSCAPE + ), + Williams + ) end optp diff --git a/librubyfmt/src/render_targets.rs b/librubyfmt/src/render_targets.rs index c110103e..c95233a5 100644 --- a/librubyfmt/src/render_targets.rs +++ b/librubyfmt/src/render_targets.rs @@ -284,7 +284,13 @@ impl AbstractTokenTarget for BreakableCallChainEntry { 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(); } } } @@ -298,18 +304,10 @@ impl AbstractTokenTarget for BreakableCallChainEntry { { 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 the last breakable is multiline (and not a block), ignore it. The user likely + // intentionally chose a line break strategy, so try our best to respect it if let Some(AbstractLineToken::BreakableEntry(be)) = tokens.last() { - // For block params, always pop it if it's multiline, otherwise we'd *always* multiline the whole block regardless of the contents. - // Never pop brace blocks, since we've already cleared their contents above, so now we're only looking at the params, which are still relevant. - if (be.delims != BreakableDelims::for_block_params() || be.is_multiline()) - && be.delims != BreakableDelims::for_brace_block() - { + if be.is_multiline() && be.delims != BreakableDelims::for_brace_block() { tokens.pop(); } } From 3137316b549124b6d86bc7916009c01ea86f316a Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Thu, 22 Feb 2024 00:26:03 +0000 Subject: [PATCH 5/6] Try and make single-call breakables less ocassionally-gross --- .../small/block_param_line_length_expected.rb | 5 +- .../breakables_over_line_length_expected.rb | 6 +- .../small/heredoc_method_call_expected.rb | 61 +++++++++---------- fixtures/small/long_blockvar_expected.rb | 25 ++++---- .../small/multiline_chain_in_block_actual.rb | 5 ++ .../multiline_chain_in_block_expected.rb | 10 +++ librubyfmt/src/render_targets.rs | 10 ++- 7 files changed, 72 insertions(+), 50 deletions(-) diff --git a/fixtures/small/block_param_line_length_expected.rb b/fixtures/small/block_param_line_length_expected.rb index 535a5a6e..4abaa75c 100644 --- a/fixtures/small/block_param_line_length_expected.rb +++ b/fixtures/small/block_param_line_length_expected.rb @@ -20,5 +20,6 @@ .quux .what_comes_after_quux_idk_aaaahhhhhhh .map(&:foo) -ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls::ThisIsAReallyLongClassName::ButSlightShorter - .new(foo: bar_baz_quuz) +ThisIsAReallyLongClassName::ButSlightShorterWithMoreCalls::ThisIsAReallyLongClassName::ButSlightShorter.new( + foo: bar_baz_quuz +) diff --git a/fixtures/small/breakables_over_line_length_expected.rb b/fixtures/small/breakables_over_line_length_expected.rb index 79b4d17a..6d4da475 100644 --- a/fixtures/small/breakables_over_line_length_expected.rb +++ b/fixtures/small/breakables_over_line_length_expected.rb @@ -18,7 +18,9 @@ ReallyLongThing ] -if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample - .calls_a_thing(a, b) +if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample.calls_a_thing( + a, + b + ) puts("") end diff --git a/fixtures/small/heredoc_method_call_expected.rb b/fixtures/small/heredoc_method_call_expected.rb index 324347ea..903e4ff9 100644 --- a/fixtures/small/heredoc_method_call_expected.rb +++ b/fixtures/small/heredoc_method_call_expected.rb @@ -1,35 +1,34 @@ class William::Carlos::Williams - landscape_with_the_fall_of_icarus = T - .let( - new( - <<~LANDSCAPE - According to Brueghel - when Icarus fell - it was spring - - a farmer was ploughing - his field - the whole pageantry - - of the year was - awake tingling - with itself - - sweating in the sun - that melted - the wings' wax - - unsignificantly - off the coast - there was - - a splash quite unnoticed - this was - Icarus drowning - LANDSCAPE - ), - Williams - ) + landscape_with_the_fall_of_icarus = T.let( + new( + <<~LANDSCAPE + According to Brueghel + when Icarus fell + it was spring + + a farmer was ploughing + his field + the whole pageantry + + of the year was + awake tingling + with itself + + sweating in the sun + that melted + the wings' wax + + unsignificantly + off the coast + there was + + a splash quite unnoticed + this was + Icarus drowning + LANDSCAPE + ), + Williams + ) end optp diff --git a/fixtures/small/long_blockvar_expected.rb b/fixtures/small/long_blockvar_expected.rb index d85153de..11f86030 100644 --- a/fixtures/small/long_blockvar_expected.rb +++ b/fixtures/small/long_blockvar_expected.rb @@ -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 diff --git a/fixtures/small/multiline_chain_in_block_actual.rb b/fixtures/small/multiline_chain_in_block_actual.rb index 95bee5bd..5c6fdc0e 100644 --- a/fixtures/small/multiline_chain_in_block_actual.rb +++ b/fixtures/small/multiline_chain_in_block_actual.rb @@ -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 diff --git a/fixtures/small/multiline_chain_in_block_expected.rb b/fixtures/small/multiline_chain_in_block_expected.rb index 7303c03d..2211b250 100644 --- a/fixtures/small/multiline_chain_in_block_expected.rb +++ b/fixtures/small/multiline_chain_in_block_expected.rb @@ -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 diff --git a/librubyfmt/src/render_targets.rs b/librubyfmt/src/render_targets.rs index c95233a5..d02125a1 100644 --- a/librubyfmt/src/render_targets.rs +++ b/librubyfmt/src/render_targets.rs @@ -304,10 +304,14 @@ impl AbstractTokenTarget for BreakableCallChainEntry { { tokens.pop(); } - // If the last breakable is multiline (and not a block), ignore it. The user likely - // intentionally chose a line break strategy, so try our best to respect it + 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 be.is_multiline() && be.delims != BreakableDelims::for_brace_block() { + if (call_count == 1 || be.is_multiline()) && be.delims != BreakableDelims::for_brace_block() && be.delims != BreakableDelims::for_block_params() { tokens.pop(); } } From a52a3717a22ad90e246695c68ea478b57d5ba4cc Mon Sep 17 00:00:00 2001 From: Reese Williams Date: Thu, 22 Feb 2024 16:14:42 +0000 Subject: [PATCH 6/6] fmt --- librubyfmt/src/render_targets.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/librubyfmt/src/render_targets.rs b/librubyfmt/src/render_targets.rs index d02125a1..8041782c 100644 --- a/librubyfmt/src/render_targets.rs +++ b/librubyfmt/src/render_targets.rs @@ -304,14 +304,27 @@ impl AbstractTokenTarget for BreakableCallChainEntry { { tokens.pop(); } - let call_count = tokens.iter().filter(|t| matches!(t, AbstractLineToken::ConcreteLineToken(ConcreteLineToken::Dot | ConcreteLineToken::LonelyOperator))).count(); + 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() { + if (call_count == 1 || be.is_multiline()) + && be.delims != BreakableDelims::for_brace_block() + && be.delims != BreakableDelims::for_block_params() + { tokens.pop(); } }