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 3 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
7 changes: 7 additions & 0 deletions fixtures/small/block_param_line_length_actual.rb
Original file line number Diff line number Diff line change
@@ -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
}
14 changes: 14 additions & 0 deletions fixtures/small/block_param_line_length_expected.rb
Original file line number Diff line number Diff line change
@@ -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
}
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
47 changes: 38 additions & 9 deletions librubyfmt/src/render_targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,37 +252,66 @@ 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()];
}
}
}
}
}

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,
Expand Down
Loading