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 enforcement for breakables #397

Merged
merged 2 commits into from
Dec 21, 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
4 changes: 3 additions & 1 deletion fixtures/large/rspec_mocks_proxy_expected.rb
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,9 @@ def initialize(source_space, *args)
# That's what this method (together with `original_unbound_method_handle_from_ancestor_for`)
# does.
def original_method_handle_for(message)
unbound_method = superclass_proxy && superclass_proxy.original_unbound_method_handle_from_ancestor_for(message.to_sym)
unbound_method = superclass_proxy && superclass_proxy.original_unbound_method_handle_from_ancestor_for(
message.to_sym
)

return super unless unbound_method
unbound_method.bind(object)
Expand Down
7 changes: 7 additions & 0 deletions fixtures/small/breakables_over_line_length_actual.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Nested breakables over the max line length
# (Sung to the tune of "Carol of the Bells")
[ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, [ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing], ReallyLongThing, ReallyLongThing]

if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample.calls_a_thing(a, b)
puts ""
end
26 changes: 26 additions & 0 deletions fixtures/small/breakables_over_line_length_expected.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Nested breakables over the max line length
# (Sung to the tune of "Carol of the Bells")
[
ReallyLongThing,
ReallyLongThing,
ReallyLongThing,
ReallyLongThing,
[
ReallyLongThing,
ReallyLongThing,
ReallyLongThing,
ReallyLongThing,
ReallyLongThing,
ReallyLongThing,
ReallyLongThing
],
ReallyLongThing,
ReallyLongThing
]

if Opus::Foo::Bar::Baz::ReallyLongName::Example::ExampleExampleExampleExampleExampleExampleExampleExample.calls_a_thing(
a,
b
)
puts("")
end
4 changes: 4 additions & 0 deletions librubyfmt/src/delimiters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,8 @@ impl BreakableDelims {
contents: self.multi_line.close.clone(),
}
}

pub fn single_line_len(&self) -> usize {
self.single_line.open.len() + self.single_line.close.len()
}
}
11 changes: 11 additions & 0 deletions librubyfmt/src/intermediary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,17 @@ impl Intermediary {
self.tokens
}

pub fn current_line_length(&self) -> usize {
if self.tokens.is_empty() {
return 0;
}

let line_start_index = self.index_of_last_hard_newline + 1;
let tokens_on_current_line = &self.tokens[line_start_index..];

tokens_on_current_line.iter().map(|t| t.len()).sum()
}

pub fn push(&mut self, lt: ConcreteLineToken) {
self.debug_assert_newlines();
let mut do_push = true;
Expand Down
28 changes: 28 additions & 0 deletions librubyfmt/src/line_tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,34 @@ impl ConcreteLineToken {
}
}

/// The length of the token's string representation
pub fn len(&self) -> usize {
use ConcreteLineToken::*;
// The alternative to this match condition would be to clone and render
// each individual string token, which would increase the allocations of rubyfmt
// by an order of magnitude
match self {
AfterCallChain => 0, // purely semantic token, doesn't render
Indent { depth } => *depth as usize,
Keyword { keyword: contents }
| Op { op: contents }
| DirectPart { part: contents }
| LTStringContent { content: contents }
| Comment { contents }
| Delim { contents }
| ConditionalKeyword { contents }
| HeredocClose { symbol: contents }
| ModKeyword { contents } => contents.len(),
HardNewLine | Comma | Space | Dot | OpenSquareBracket | CloseSquareBracket
| OpenCurlyBracket | CloseCurlyBracket | OpenParen | CloseParen | SingleSlash => 1,
DoKeyword | CommaSpace | LonelyOperator | ColonColon | DoubleQuote => 2,
DefKeyword | Ellipsis | End => 3, // "def"/"..."/"end"
ClassKeyword => 5, // "class"
ModuleKeyword => 6, // "module"
DataEnd => 7, // "__END__"
}
}

fn is_block_closing_token(&self) -> bool {
match self {
Self::End => true,
Expand Down
2 changes: 1 addition & 1 deletion librubyfmt/src/render_queue_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ impl RenderQueueWriter {
}

fn format_breakable_entry(accum: &mut Intermediary, be: BreakableEntry) {
let length = be.single_line_string_length();
let length = accum.current_line_length() + be.single_line_string_length();

if (length > MAX_LINE_LENGTH || be.is_multiline())
&& be.entry_formatting_context() != FormattingContext::StringEmbexpr
Expand Down
3 changes: 2 additions & 1 deletion librubyfmt/src/render_targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ impl AbstractTokenTarget for BreakableEntry {
.iter()
.map(|tok| tok.clone().into_single_line())
.map(|tok| tok.into_ruby().len())
.sum()
.sum::<usize>()
+ self.delims.single_line_len()
}

fn push_line_number(&mut self, number: LineNumber) {
Expand Down