[T-650] Fix number padding in codeframe being incorrect if endLine ==…

… 10 (#4874)

* improve codeframe tests

* more tweaks

* remove .only

* add a failing unit test for the issue

* fix number padding

* padStart instead of end

* fix lint

* get rid of proprietary wordpress example

.eslintignore

@@ -23,3 +23,4 @@ dist
 coverage
 node_modules
 tmp
+fixtures

packages/core/codeframe/src/codeframe.js

@@ -71,15 +71,15 @@ export default function codeFrame(
   // Prefix lines with the line number
   const lineNumberPrefixer = (params: {|
     lineNumber?: string,
-    endLine: string,
+    lineNumberLength: number,
     isHighlighted: boolean,
   |}) => {
-    let {lineNumber, endLine, isHighlighted} = params;
+    let {lineNumber, lineNumberLength, isHighlighted} = params;
 
     return `${isHighlighted ? highlighter('>') : ' '} ${
       lineNumber
-        ? lineNumber.padEnd(endLine.length, ' ')
-        : ' '.repeat(endLine.length)
+        ? lineNumber.padStart(lineNumberLength, ' ')
+        : ' '.repeat(lineNumberLength)
     } | `;
   };
 
@@ -111,12 +111,13 @@ export default function codeFrame(
   // Calculate first and last line index of codeframe
   let startLine = firstHighlight.start.line - opts.padding.before;
   startLine = startLine < 0 ? 0 : startLine;
-  let endLine = lastHighlight.end.line + opts.padding.after;
-  endLine =
-    endLine - startLine > opts.maxLines
+  let endLineIndex = lastHighlight.end.line + opts.padding.after;
+  endLineIndex =
+    endLineIndex - startLine > opts.maxLines
       ? startLine + opts.maxLines - 1
-      : endLine;
-  let endLineString = endLine.toString(10);
+      : endLineIndex;
+
+  let lineNumberLength = (endLineIndex + 1).toString(10).length;
 
   // Split input into lines and highlight syntax
   let lines = code.split(NEWLINE);
@@ -134,7 +135,7 @@ export default function codeFrame(
     currentLineIndex < syntaxHighlightedLines.length;
     currentLineIndex++
   ) {
-    if (currentLineIndex > endLine) break;
+    if (currentLineIndex > endLineIndex) break;
     if (currentLineIndex > syntaxHighlightedLines.length - 1) break;
 
     // Find highlights that need to get rendered on the current line
@@ -158,8 +159,8 @@ export default function codeFrame(
       );
 
     let lineLengthLimit =
-      opts.terminalWidth > endLineString.length + 7
-        ? opts.terminalWidth - (endLineString.length + 5)
+      opts.terminalWidth > lineNumberLength + 7
+        ? opts.terminalWidth - (lineNumberLength + 5)
         : 10;
 
     // Split the line into line parts that will fit the provided terminal width
@@ -189,7 +190,7 @@ export default function codeFrame(
     resultLines.push(
       lineNumberPrefixer({
         lineNumber: (currentLineIndex + 1).toString(10),
-        endLine: endLineString,
+        lineNumberLength,
         isHighlighted: lineHighlights.length > 0,
       }) + syntaxHighlightedLine,
     );
@@ -274,7 +275,7 @@ export default function codeFrame(
     if (highlightLine) {
       resultLines.push(
         lineNumberPrefixer({
-          endLine: endLineString,
+          lineNumberLength,
           isHighlighted: true,
         }) + highlightLine,
       );

packages/core/codeframe/test/codeframe.test.js

@@ -1,4 +1,6 @@
 import assert from 'assert';
+import {readFileSync} from 'fs';
+import {join as joinPath} from 'path';
 
 import codeframe from '../src/codeframe';
 
@@ -393,7 +395,7 @@ describe('codeframe', () => {
     assert.equal(lines[7], '  9 | test');
   });
 
-  it('should properly pad numbers', () => {
+  it('should properly pad numbers for large files', () => {
     let codeframeString = codeframe('test\n'.repeat(1000), [
       {
         start: {
@@ -415,17 +417,22 @@ describe('codeframe', () => {
           column: 2,
           line: 100,
         },
-        message: 'test',
+        message: 'test 2',
       },
     ]);
 
     let lines = codeframeString.split(LINE_END);
     assert.equal(lines.length, 7);
-    assert.equal(lines[0], '  98  | test');
+    assert.equal(lines[0], '   98 | test');
+    assert.equal(lines[1], '>  99 | test');
+    assert.equal(lines[2], '>     |  ^ test');
+    assert.equal(lines[3], '> 100 | test');
+    assert.equal(lines[4], '>     |  ^ test 2');
+    assert.equal(lines[5], '  101 | test');
     assert.equal(lines[6], '  102 | test');
   });
 
-  it('should properly pad numbers', () => {
+  it('should properly pad numbers for short files', () => {
     let codeframeString = codeframe('test\n'.repeat(1000), [
       {
         start: {
@@ -453,13 +460,17 @@ describe('codeframe', () => {
 
     let lines = codeframeString.split(LINE_END);
     assert.equal(lines.length, 11);
-    assert.equal(lines[0], '  6  | test');
+    assert.equal(lines[0], '   6 | test');
+    assert.equal(lines[4], '   9 | test');
+    assert.equal(lines[5], '  10 | test');
+    assert.equal(lines[6], '  11 | test');
     assert.equal(lines[10], '  14 | test');
   });
 
   it('should properly use maxLines', () => {
+    let line = 'test '.repeat(100);
     let codeframeString = codeframe(
-      'test\n'.repeat(100),
+      `${line}\n`.repeat(100),
       [
         {
           start: {
@@ -487,14 +498,16 @@ describe('codeframe', () => {
       {
         useColor: false,
         maxLines: 10,
+        terminalWidth: 5,
       },
     );
 
     let lines = codeframeString.split(LINE_END);
     assert.equal(lines.length, 13);
-    assert.equal(lines[0], '  4  | test');
-    assert.equal(lines[11], '> 13 | test');
-    assert.equal(lines[12], '>    | ^^^^');
+    assert.equal(lines[0], '   4 | test test ');
+    assert.equal(lines[7], '  10 | test test ');
+    assert.equal(lines[11], '> 13 | test test ');
+    assert.equal(lines[12], '>    | ^^^^^^^^^^');
   });
 
   it('should be able to handle tabs', () => {
@@ -741,4 +754,46 @@ describe('codeframe', () => {
     assert.equal(lines[2], '> 2 | ew line new line ne');
     assert.equal(lines[3], '>   | ^^^^^^ I have a message');
   });
+
+  it('Should pad properly, T-650', () => {
+    let fileContent = readFileSync(
+      joinPath(__dirname, './fixtures/a.js'),
+      'utf8',
+    );
+    let codeframeString = codeframe(
+      fileContent,
+      [
+        {
+          start: {
+            line: 8,
+            column: 10,
+          },
+          end: {
+            line: 8,
+            column: 48,
+          },
+        },
+      ],
+      {
+        useColor: false,
+        syntaxHighlighting: false,
+        language: 'js',
+        terminalWidth: 100,
+      },
+    );
+
+    let lines = codeframeString.split(LINE_END);
+    assert.equal(lines.length, 5);
+    assert.equal(lines[0], `   7 | import Tooltip from '../tooltip';`);
+    assert.equal(
+      lines[1],
+      `>  8 | import VisuallyHidden from '../visually-hidden';`,
+    );
+    assert.equal(
+      lines[2],
+      '>    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^',
+    );
+    assert.equal(lines[3], '   9 | ');
+    assert.equal(lines[4], '  10 | /**');
+  });
 });

packages/core/codeframe/test/fixtures/a.js

@@ -0,0 +1,13 @@
+import test from 'test';
+import component from './component';
+
+/**
+ * This is a comment
+ */
+import Tooltip from '../tooltip';
+import VisuallyHidden from '../visually-hidden';
+
+/**
+ * This is another comment
+ */
+import {Label} from './label';