Handle very long lines

[karreiro]

Fixes #5

This PR updates the ErrorHighlight::DefaultFormatter to handle long lines. We're now identifying the number of columns in the terminal so that we can truncate the snippet and present it on a single line.

Before

After

[mame]

@karreiro Thanks for the PR. I approved the CI run, so could you please make the CI pass?

Regard of the code, it looks like it does not handle the case where an error occurred in the middle of a long line. (If I am wrong, sorry.) In such a case, I think it has to add two ellipses not only at the beggining but also at the end.

Another thing is a matter of taste, but I prefer to manipulate snippet and last_column directly in message_for method rather than adding a method that returns a modified spot hash. I can do this myself after the merge.

[karreiro]

Thank you for the review, @mame! The CI should be passing now :)

Regard of the code, it looks like it does not handle the case where an error occurred in the middle of a long line. (If I am wrong, sorry.) In such a case, I think it has to add two ellipses not only at the beggining but also at the end.

That's correct. I initially thought the ...<error> case would cover that, but that's not accurate indeed. I've added the logic to handle this scenario and included some extra unit tests.

Another thing is a matter of taste, but I prefer to manipulate snippet and last_column directly in message_for method rather than adding a method that returns a modified spot hash. I can do this myself after the merge.

That's totally fair, the code base needs to be consistent with the code style. I've made this change, and now the logic is in the message_for method as you suggested.

Thanks again for the review!

[mame]

Thanks. I think it's getting better.

I actually tried your patch and noticed many things. They are all pretty minor, but I write down everthing I noticed.

Should not truncate the snippet by default when STDERR is not a tty

Currently, this patch changes the output of ruby test.rb 2> stderr_log depending on the terminal size. I think it is strange.

Maybe terminal_columns should check if STDERR.tty?. And if it is a tty, it should use STDERR.winsize[1] instead of IO.console.winsize[1]. Otherwise, it should stop truncating the snippet.

Opt-out of truncating

If you set ErrorHighlight::DefaultFormatter.viewport_size = nil, I think it should stop truncating.

The method name viewport_size

I don't feel comfortable to call this method viewport_size because it only handles width or column size. snippet_max_width or something is better?

Handling of extremely small size

Such as viewport_size = 3, produces the following output.

......
 ^^^^^

I think we should have the minimum limit for the width. (I guess it should at least seven, but I am not sure what value is appropriate.)
And maybe we should raise an exception if lower size is specified than the limit, or automatically increase to the limit with a warning. (Not sure which is better.)

The ellipsis at the beginning is not so preferable

The current implementation tries to put the left edge of the underline at the center of the line.
However, I think it would better to avoid the ellipsis at the beginning if possible.

For example, your test includes:

...000000000000000000000000000000000 + 1.time { 10000000000000000000000000000...
                                        ^^^^^

For this particular case, the following looks to me easier to read, even if the underline is not at the center.

    100000000000000000000000000000000000000 + 1.time { 1000000000000000000000...
                                               ^^^^^

However, if the underline position is too close to the right edge, as in the following, the ellipsis at the beginning may look better.

    10000000000000000000000000000000000000000000000000000000000000 + 1.time {...
                                                                      ^^^^^

Only when the right edge of the underline is "too close" (need to decide a good threshold) to the right edge of the viewport, the left edge of the underline should be centered by omitting the beginning of the line, I guess.

I'm sorry for being really detailed.

[mame]

pp.rb had rescue LoadError, NoMethodError, SystemCallError for require "io/console". This rescue guard would be needed for error_highlight as well.

https://github.com/ruby/ruby/blob/d58ec11945a1f8bd26bb6d61a7e2c2f983350930/lib/pp.rb#L79-L84

Also, pp.rb falls back to ENV["COLUMNS"] or default 80 on failure.
I am not sure if this is useful for error_highlight. I guess it would be safe to stop truncating if require fails.

[karreiro]

I'm sorry for being really detailed.

Please, no worries at all — this level of detail is fantastic! :)

I've implemented most of the suggestions in the latest commit, but I have a few minor questions to ensure the PR is nice :)

--

Should not truncate the snippet by default when STDERR is not a tty

I've made this change, and it works as expected now!

--

Opt-out of truncating

I went ahead and implemented this, but I wonder if a slightly more intuitive API might be something like this:

ErrorHighlight::DefaultFormatter.truncate
# => true

ErrorHighlight::DefaultFormatter.truncate = false
# => false

What do you think?

--

The method name viewport_size

I renamed it to snippet_max_width as suggested. Personally, I think truncate_size or truncate_col could also work. What do you think? (depending of the suffix, we may apply that in the rest of the variables as well)

--

Handling of extremely small size

Raising an exception seems a bit severe, so I opted to automatically adjust the limit as suggested. However, I found 7 to be too small for this context. Here's an example with 7 characters being truncated:

.......
   ^------ this is the dot from the method call

Instead, I've set a minimum column size of 20, as it allows for more meaningful output:

...000 + 1.time {...

--

The ellipsis at the beginning is not so preferable

Totally agree. I've updated this so the ellipsis only appears when the last column doesn’t fit in the window.

--

pp.rb had rescue LoadError, NoMethodError, SystemCallError for require "io/console". This rescue guard would be needed for error_highlight as well.

Good catch! We're now handling those errors, and I also agree with the decision to stop truncating in those cases.

--

Thanks again for all the great input!

[mame]

Thank you very much! It looks awesome. Just for the case, let me test it actually in the next week.

Personally, I think truncate_size or truncate_col could also work. What do you think?

They sound clearer to me than viewport_size. I can't decide if they are better than snippet_max_width. I'll ask Matz next time.

(depending of the suffix, we may apply that in the rest of the variables as well)

Sorry, I couldn't get this meaning of this. What do you mean by "the rest of the variables"?

[karreiro]

Thank you very much! It looks awesome. Just for the case, let me test it actually in the next week.

Thank you very much!

Sorry, I couldn't get this meaning of this. What do you mean by "the rest of the variables"?

Sorry, it wasn’t clear indeed. I was referring to not just variables, but also methods and constants.

What I meant is that if we rename snippet_max_width to something like truncate_size, truncate_column, or truncate_col, I’d also rename other related local elements like available_width, terminal_width, and MIN_SNIPPET_WIDTH accordingly.

For example, I'd rename them to available_columns, terminal_columns, and MIN_SNIPPET_COLUMN to keep the naming consistent :)

[mame]

Sorry I'm late! I tried it and it looked good, so I merged it. Thank you so much!

[mame]

Do you want me to cut a release soon? Ruby 3.4 is coming soon, so we will definitely release it within 2 months. If it is better to release it earlier for Ruby 3.3 or before, I will do it.

[mame]

Oh, I forgot about name consistency. I don't really care about the names of local variables. If I wrote it myself, I would even have used a single letter variable like n or w ;-)

Come to think of it, I wonder if max_snippet_width sounds more natural than snippet_max_width?

[mame]

I didn't like truncate_size so much because it is not clear to me that the truncation subject is a snippet and that it limits a horizontal size (width, column).
In this respect, truncate_col is better because it implies a horizontal size, but the truncation subject is still not clear (to me).
(I wonder if ErrorHighlight::DefaultFormatter.truncate_col is sufficient? Hmm)

[karreiro]

Thanks so much for all the feedback and improvements, @mame!

Ruby 3.4 looks good! It'll be awesome to see this enhancement there :)

I wonder if max_snippet_width sounds more natural than snippet_max_width ?

I agree that max_snippet_width feels more natural since it follows the usual adjective-noun pattern.

And from the perspective of truncation, max_snippet_width feels a bit clearer than truncate_col indeed.

I've made that change here:
#52 :)