Truncated view for blocks longer than 10 lines #1168

anuragsoni · 2018-08-22T00:29:30Z

Signed-off-by: Anurag Soni anuragsoni.13@gmail.com

anuragsoni · 2018-08-22T00:30:53Z

@emillon Just to start the discussion about what will be suitable for stanzas longer than 10 lines.

emillon · 2018-08-22T07:44:55Z

I saw that elm and bucklescript both use | as a separator before lines - I agree that it looks better (maybe with a space before).

I had a look at how bucklescript does its formatting (see also these tests) and the main points are:

it truncates the middle instead of the end
it includes some extra context lines
it trims leading whitespace, if any

I don't think (2) and (3) are very useful for now, but (1) seems interesting. What are your thoughts on that?

anuragsoni · 2018-08-22T13:19:36Z

I agree about (2). I don't think extra context lines will help too much when it comes to these fields as we are able to show the exact stanza that has a problem in most cases.

For (3) I'm guessing at some point we might be able to do the same thing that dune unstable-fmt does for formatting?

I like the idea of truncating the middle portions as the trailing ... isn't easy to see. So it'd be like following:

File "dune", line 3, characters 13-192:
   3 |   (libraries (a
   4 |               b
   5 |               c
   6 |               d
   7 |               e
   8 |               f
   9 |               g

-----

  20 |               h
  31 |               i
  32 |               j)

emillon · 2018-08-23T14:24:25Z

Yes, that seems right. I agree that there'll probably be a nice synergy with dune fmt at some point.

anuragsoni · 2018-08-24T01:53:34Z

@emillon I've tried to come up with an approach to the 1st point discussed above (truncating the portions in the middle)

emillon · 2018-08-24T09:58:51Z

test/blackbox-tests/test-cases/too-many-parens/run.t

+  3 |   (libraries (a
+  4 |               b
+  5 |               c
+  6 |               d


I think that this should be padded with one extra space so that the vertical bars line up with below (this file is a great test 👍 ).

Ah good catch. I think the problem was that I need to pad the first block of lines with the padding width from the last block of lines for them to line up. I hadn't handled the case that the first four lines might have less digits than the last four 😄

I've updated to take that into account

emillon · 2018-08-24T10:00:57Z

test/blackbox-tests/test-cases/too-many-parens/run.t

+  5 |               c
+  6 |               d
+
+  ------


That's probably a bit too large, can you align it to just before the position of | as in your comment before? I'm also curious about using ... since that's more associated with cutting parts out (and that's what bucklescript uses).

Good point. I've updated to use ... That indeed looks nicer. Also they should now be aligned with the |

emillon · 2018-08-24T12:30:35Z

test/blackbox-tests/test-cases/too-many-parens/run.t

  $ dune build --root d
  File "dune", line 3, characters 13-192:
+   3 |   (libraries (a


I notice that the truncated view always displays 11 lines. This means that 10 lines will be displayed in full, but 11 lines will be displayed with 3 lines missing, while keeping the same number of lines. It's a bit unfortunate. I can see three things that we can do:

increase the threshold (display more messages in full)

remove some context lines (e.g. switch to 3)

remove the empty lines

I think that 3 context lines would be enough, but that's up to you.

I'd think that there will always be a scenario where we transition from a full stanza to truncated view where we'd see less lines in the truncated version. I think a combination of increasing the threshold and removing some context lines (I agree that 3 should be enough) will be a good start. I'll update later today

emillon · 2018-08-24T12:33:35Z

src/errors.ml

+          let print_lines ?(trailing_line=false) lines padding_width =
+            List.iter ~f:(fun (lnum, l) ->
+              Format.fprintf pp "%*s | %s\n" padding_width lnum l) lines;
+            if trailing_line then


That boolean parameter is control coupling. You can remove it by extracting a print_ellipsis function and calling it between the two print_line call sites below.

good catch. I'll update. It's a leftover from a version I had when I wasn't passing this function the extracted lines

emillon · 2018-08-24T12:36:15Z

src/errors.ml

+              let line = String.make (padding_width + 2) '.' in
+              Format.fprintf pp "\n%s\n\n" line
+          in
+          if num_lines <= 10 then


What do you think about extracting this constant out of this function ? Like let maximum_lines_to_print_in_full or something. Same goes for the 3 below, you can pull out a let context_lines = 4

good point. I'll address this

emillon · 2018-08-24T12:36:42Z

src/errors.ml

+            (* We need to send the padding width from the last four lines
+               so the two blocks of lines align if they have different number
+               of digits in their line numbers *)
+            let first_four = file_lines path ~start:(start.pos_lnum)


these names are a bit too specific - especially if they end up being 3 lines :)

I agree. I couldn't think of any better names at that point since start and stop were already taken. I'll rename this 😄

emillon · 2018-08-24T12:37:53Z

src/errors.ml

+        else
+          let get_padding lines =
+            let last_lnum = Option.map ~f:fst (List.last lines) in
+            Option.value_exn (Option.map ~f:String.length last_lnum)


If you call value_exn on the first option, you'll be able to remove the second Option.map.

emillon · 2018-08-24T12:42:17Z

src/errors.ml

-          List.iter ~f:(fun (lnum, l) ->
-            Format.fprintf pp "%*s: %s\n" padding_width lnum l)
-          lines
+        else


We can solve this after the other comments, but I believe that it's the point where this function has enough logic in it. It should be possible to extract a type that determines how to print a file part:

type display_style = | No_display | One_line of int (* line number *) | Block of int * int (* start, stop *)

Then it's possible to split the large function into one that determines the display style, and one function per display style. As a bonus, things like determining the padding size can be pulled out and factored a bit.

What do you think about that?

I like the idea. I'll try this after I address the other comments!

emillon · 2018-08-31T12:41:38Z

Hi Anurag, just checking on this - there's no rush but is there anything that I can help you with?

anuragsoni · 2018-08-31T12:50:12Z

Hi! I should have replied earlier!! I had pushed some changes to address all comments apart from adding the new type. I was hoping to get some feedback on that before I refactor to create a new type!

I figured you were busy so thought i'd wait 😄

emillon · 2018-08-31T13:13:09Z

Ah, it's good that I checked then :) The changes look good, you can have a go at the new type if you're still up for it!

anuragsoni · 2018-08-31T13:17:50Z

I will definitely try to refactor with a new type. I'll be on vacation for a few days so should be able to update with the new type sometime next week. Also, I can update this branch with the latest master later today if you prefer, so there can be a new PR with the refactor to a new type that should be transparent change for the implementation

emillon · 2018-08-31T14:23:43Z

As you prefer; either we can merge this now or after, with the refactoring.

anuragsoni · 2018-08-31T14:36:30Z

I will update this PR later today after syncing this with master. Maybe we can merge this before to avoid conflicts? The refactor should be transparent to all tests so I can do that next week (we can have a separate issue for that, and I can pick that up)

What do you think?

emillon · 2018-08-31T14:38:27Z

That works for me!

anuragsoni · 2018-08-31T23:54:07Z

@emillon i've updated the PR to be in sync with the current master.

rgrinberg · 2018-09-01T07:55:38Z

@anuragsoni make sure to remove the WIP from the title when this is ready

anuragsoni · 2018-09-01T12:38:50Z

@rgrinberg done! I don't have access to a computer this weekend so I can't rebase with current master

emillon · 2018-09-03T07:26:14Z

Great, I'll rebase and merge. Thanks!

Signed-off-by: Anurag Soni <anuragsoni.13@gmail.com>

anuragsoni changed the title ~~Truncated view for blocks longer than 10 lines~~ [WIP] Truncated view for blocks longer than 10 lines Aug 22, 2018

anuragsoni requested a review from a user August 24, 2018 01:52

anuragsoni requested review from emillon and rgrinberg as code owners August 24, 2018 01:52

anuragsoni changed the title ~~[WIP] Truncated view for blocks longer than 10 lines~~ Truncated view for blocks longer than 10 lines Aug 24, 2018

emillon reviewed Aug 24, 2018

View reviewed changes

anuragsoni changed the title ~~Truncated view for blocks longer than 10 lines~~ [WIP] Truncated view for blocks longer than 10 lines Aug 24, 2018

anuragsoni mentioned this pull request Aug 31, 2018

Always display the line number in file excerpts #1203

Merged

anuragsoni changed the title ~~[WIP] Truncated view for blocks longer than 10 lines~~ Truncated view for blocks longer than 10 lines Sep 1, 2018

Truncated view for blocks longer than 10 lines

81797f8

Signed-off-by: Anurag Soni <anuragsoni.13@gmail.com>

emillon merged commit 8dbfd66 into ocaml:master Sep 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Truncated view for blocks longer than 10 lines #1168

Truncated view for blocks longer than 10 lines #1168

anuragsoni commented Aug 22, 2018

anuragsoni commented Aug 22, 2018

emillon commented Aug 22, 2018

anuragsoni commented Aug 22, 2018 •

edited

Loading

emillon commented Aug 23, 2018

anuragsoni commented Aug 24, 2018

emillon Aug 24, 2018

anuragsoni Aug 24, 2018 •

edited

Loading

emillon Aug 24, 2018

anuragsoni Aug 24, 2018

emillon Aug 24, 2018

anuragsoni Aug 24, 2018

emillon Aug 24, 2018

anuragsoni Aug 24, 2018

emillon Aug 24, 2018

anuragsoni Aug 24, 2018

emillon Aug 24, 2018

anuragsoni Aug 24, 2018

emillon Aug 24, 2018

emillon Aug 24, 2018

anuragsoni Aug 24, 2018

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

rgrinberg commented Sep 1, 2018

anuragsoni commented Sep 1, 2018

emillon commented Sep 3, 2018

+|               c
+|               d
+                ------

Truncated view for blocks longer than 10 lines #1168

Truncated view for blocks longer than 10 lines #1168

Conversation

anuragsoni commented Aug 22, 2018

anuragsoni commented Aug 22, 2018

emillon commented Aug 22, 2018

anuragsoni commented Aug 22, 2018 • edited Loading

emillon commented Aug 23, 2018

anuragsoni commented Aug 24, 2018

Choose a reason for hiding this comment

anuragsoni Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

emillon commented Aug 31, 2018

anuragsoni commented Aug 31, 2018

rgrinberg commented Sep 1, 2018

anuragsoni commented Sep 1, 2018

emillon commented Sep 3, 2018

anuragsoni commented Aug 22, 2018 •

edited

Loading

anuragsoni Aug 24, 2018 •

edited

Loading