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

Max line length isn't consistently respected #379

Closed
reese opened this issue Dec 1, 2022 · 2 comments · Fixed by #452
Closed

Max line length isn't consistently respected #379

reese opened this issue Dec 1, 2022 · 2 comments · Fixed by #452

Comments

@reese
Copy link
Collaborator

reese commented Dec 1, 2022

  • Ruby version: 2.7.2
  • Rubyfmt git sha: 4d090e0

Input file

if Some::ReallyLongClassName::GoesHere::LetsMakeItEvenLonger.who_framed_roger_rabbit?(type: opts[:type], file_type: opts[:file_type])
end

Rubyfmt's output

if Some::ReallyLongClassName::GoesHere::LetsMakeItEvenLonger.who_framed_roger_rabbit?(type: opts[:type], file_type: opts[:file_type])
end

This breaks because

This goes beyond the standard max line length of 120 characters (it's 133).

This is an inherent limitation in the way we've currently handled breakables, and I think we need to do a big ol' refactor to make this more consistent across the board, since it ends up breaking in several places to varying degrees. The short version is that in most cases, we check whether the breakable itself is longer than the line length limit, not whether it ends up passing the line limit once it's been rendered. This means in practice, we're not actually applying a line length limit, but rather a breakable length limit.

I haven't really dug into what the exact implementation for this would look like, but I think we'd end up doing something like this: during the render queue, track the line length of the current line and reset it at every hard newline, and then attempt to render breakables as single line and breaking if they go over. However, we'll have to be careful here -- we have to be sure that once we go to multiline, we still correctly track the line length inside that breakable, because breakables nested inside of it may also need to be further broken down. Take this as as an example:

[ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, [ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing], ReallyLongThing, ReallyLongThing]

We've got two arrays, where the inner array is 119 characters long without including whitespace. However, since the outer array will render to multiline, the inner array will be indented two characters in, making the line 121 characters long, so it should break to multiline, but it doesn't currently.

Current output:

[
  ReallyLongThing,
  ReallyLongThing,
  ReallyLongThing,
  ReallyLongThing,
  [ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing, ReallyLongThing],
  ReallyLongThing,
  ReallyLongThing
]

Expected output:

[
  ReallyLongThing,
  ReallyLongThing,
  ReallyLongThing,
  ReallyLongThing,
  [
    ReallyLongThing,
    ReallyLongThing,
    ReallyLongThing,
    ReallyLongThing,
    ReallyLongThing,
    ReallyLongThing,
    ReallyLongThing
  ],
  ReallyLongThing,
  ReallyLongThing
]

FWIW, this may also mean refactoring a bit of how we do some other "fake breakables"[0] like method chains and binary operators, but just getting this to work for breakables would be a great start.


[0] it would be great to extend them to be actual breakables, but blocks make that a bit difficult -- although not impossible

@reese
Copy link
Collaborator Author

reese commented Dec 20, 2022

I've got a working example for breakables, but here's a test case for "fake breakables" that we should tackle later later:

if Opus::Foo::Bar::Baz::ReallyLongName::Example::OhHiIDidntSeeYouThere.map { _1.do_stuff }.select { _1.is_cool? }.collect
end

The first line is 121 characters long and should break, but it currently doesn't because the call chain itself is under 120.

@reese
Copy link
Collaborator Author

reese commented Dec 8, 2023

The last test case that I know of here -- and the last thing I want to fix before closing this ticket -- is binary operators. An example of this working incorrectly would be

some_value_goes_here = if some_really_long_name_so_that_this_example_breaks_the_way_that_i_want_it_to && foo.some_other_condition? && other_thing
    ""
end

which first will break on the call chain

some_value_goes_here = if some_really_long_name_so_that_this_example_breaks_the_way_that_i_want_it_to && foo
    .some_other_condition? && other_thing
  ""
end

and then will break as a multiline binary op

some_value_goes_here = if some_really_long_name_so_that_this_example_breaks_the_way_that_i_want_it_to &&
    foo
      .some_other_condition? &&
    other_thing
  ""
end

This should probably actually just break on the individual expressions on the first try, e.g.

some_value_goes_here = if some_really_long_name_so_that_this_example_breaks_the_way_that_i_want_it_to &&
    foo.some_other_condition? &&
    other_thing
  ""
end

I think we can do this by also making binary chains a breakable target, because at the moment they're very much hacked together to handle multilining, but that implementation doesn't handle line length breaks very well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant