[RFE] Dynamic Reports - Incident: Add line numbers to the codeSnip string (right-aligned)

[mturley]

When rendering the codeSnip string of an Incident in the UI, we display line numbers from the source file in a gutter to the side of the code snippet. In order to align these numbers correctly and correctly mark the actual offending line, we need to know how much context is provided in the code snippet around the actual line numbered by the Incident's line property. I propose a codeSnipStartLine or similarly-named property, which would be the line number where the codeSnip begins.

[mturley]

cc @shawn-hurley @jortel @pranavgaikwad

[mturley]

Note that @shawn-hurley mentioned earlier that the codeSnip should start 10 lines above, but IMO the UI should not hard-code the number of context lines here so that we can decide to have the analyzer provide more/less context in the future without a UI change (also, if the offending line is within the first 10 lines of the file we need to reason about that in the UI)

[jortel]

Seems like a more holistic and clearer solution would be for the analyzer to prefix each line with the line number. That would provide value when using the analyzer outside of tackle (CLI) and when using the analysis data outside of the UI (like hub api). Also, removing the need for arithmetic to identify the offending line number would be less brittle.

[mturley]

For posterity, from discussion on slack with @jortel:

If we prefix each line with the line number in the codeSnip string, that is fine as long as the numbers are right-aligned (padding whitespace is to the left of the numbers, and the last character of each line number are all in the same column). This is because the UI needs to strip the line numbers out before running syntax highlighting on the code and rendering it in the monaco editor pane, and if the padding is to the right of the line numbers this will result in screwing up the leading whitespace / indentation of the code (since line numbers are not all the same number of characters).

To be clear, we need this:

 1    public class SwapNumbers {
 2        public static void main(String[] args) {
 3            float first = 1.20f, second = 2.45f;
 4  
 5            System.out.println("--Before swap--");
 6            System.out.println("First number = " + first);
 7            System.out.println("Second number = " + second);
 8  
 9            // Value of first is assigned to temporary
10            float temporary = first;
11
12            // Value of second is assigned to first
13            first = second;
14
15            // Value of temporary (which contains the initial value of first) is assigned to second
16            second = temporary;
17
18            System.out.println("--After swap--");
19            System.out.println("First number = " + first);
20            System.out.println("Second number = " + second);
21        }
22    }

and NOT this:

1     public class SwapNumbers {
2         public static void main(String[] args) {
3             float first = 1.20f, second = 2.45f;
4   
5             System.out.println("--Before swap--");
6             System.out.println("First number = " + first);
7             System.out.println("Second number = " + second);
8   
9             // Value of first is assigned to temporary
10            float temporary = first;
11
12            // Value of second is assigned to first
13            first = second;
14
15            // Value of temporary (which contains the initial value of first) is assigned to second
16            second = temporary;
17
18            System.out.println("--After swap--");
19            System.out.println("First number = " + first);
20            System.out.println("Second number = " + second);
21        }
22    }

(see the difference in the first 9 lines)

[mturley]

(I just realized I could still strip left-aligned numbers out by using the length of the last line number for all lines, but I still have a preference for right-aligned numbers as that's what editors tend to use and it looks nicer)

[shawn-hurley]

I am no opposed to this, and it does make sense we will do the same logic, but if the context number is configurable, it makes sense for that logic to live there. Note to anyone implementing this on the analyzer-lsp I am pretty sure most of our stuff is 0 indexed not 1.

[shawn-hurley]

@pranavgaikwad I think we can move this to the analyzer then (as it seems all the work is on our side now), and I believe that we should align to Demo E. What are your thoughts?

[mturley]

I am pretty sure most of our stuff is 0 indexed not 1.

@shawn-hurley do you mean the line property on an Incident is 0-indexed, and I should be showing line + 1 in the UI?

[shawn-hurley]

What our providers return is 0 indexed, I assume that you would want that to be 1 indexed. just means we have to add by one

[mturley]

Yeah I assume API users would want the same, to match what they see in their code editor

[pranavgaikwad]

Created issue to track, adding it on Demo E

[mturley]

Note for implementation (cc @pranavgaikwad): @jortel and I agreed that the code snip lines should have exactly 2 additional spaces between the line number and the code (not including any leading whitespace from the source code itself). See konveyor/tackle2-hub#414 for an example.

[rromannissen]

@mturley can we consider this done already? I see that the related PR has been merged.

[mturley]

Ah yes, this is done. Thanks @rromannissen