mookid / diffr

Yet another diff highlighting tool
MIT License
575 stars 22 forks source link

Feature Request: line numbers #21

Closed noc7c9 closed 4 years ago

noc7c9 commented 5 years ago

Not sure if this is out of scope for this project but I've always wanted line numbers for diffs instead of the @@ -xxx,y +xxx,y @@ syntax. Would be especially useful when there are large chunks.

Something like

diff --git a/src/index.js b/src/index.js
index d46a912..d34f309 100644
--- a/src/index.js
+++ b/src/index.js
@@ -13,7 +13,8 @@ const Button = ({ value, color, events }) => {
  13: };
  14: const textStyle = {
  15:     flex: 0,
- 16:     fontFamily: 'sans-serif',
+ 16:     fontFamily: 'serif',
+ 17:     color: white,
  18: };
  19: return (
  20:     <div {...events} style={containerStyle}>
mookid commented 5 years ago

@noc7c9 Thanks for the suggestion! This can be supported with a certain flag.

Out of scope would be something that involves complicated parsing; adding a line number that can be easily computed is completely reasonable. However, finding a spec of the unified diff format will confirm that the line number format is uniform between most diff tools.

However, note that in your example, line numbers after the marked lines should be duplicated, with line numbers in both the old and the new version:

diff --git a/src/index.js b/src/index.js
index d46a912..d34f309 100644
--- a/src/index.js
+++ b/src/index.js
@@ -13,7 +13,8 @@ const Button = ({ value, color, events }) => {
  13: };
  14: const textStyle = {
  15:     flex: 0,
- 16:     fontFamily: 'sans-serif',
+ 16:     fontFamily: 'serif',
+ 17:     color: white,
  17:18: };
  18:19: return (
  19:20:     <div {...events} style={containerStyle}>

I feel that this is not completely satisfying yet; let's get some time to finalize the proposal with something better before jumping to the implementation. Suggestions are welcome.

noc7c9 commented 5 years ago

The line numbers issue was something I realized when I made the issue. At the time, I decided it made the most sense to display only the new line numbers.

If you do think having both would make more sense, here's a possible formatting (on a more complex diff)

The idea is to have two columns of line numbers, old and new.

diff --git a/src/playground.js b/src/playground.js
index 33f4085..6ae6bbb 100644
--- a/src/playground.js
+++ b/src/playground.js
@@ -5,13 +5,14 @@ const heightQuery = '(min-height: 400px)';
   5: 5:
   6: 6: const widthSwitch = {
   7: 7:     '(min-width: 600px)': 5,
-  8:        '(min-width: 400px)': 4,
+     8:     '(min-width: 400px)': 4.5,
+     9:     '(min-width: 300px)': 4,
   9:10:     '(min-width: 200px)': 3,
  10:11:     default: 2,
  11:12: };
  12:13: const heightSwitch = {
  13:14:     '(min-height: 600px)': 5,
- 14:        '(min-height: 400px)': 4,
+    15:     '(min-height: 300px)': 4,
  15:16:     '(min-height: 200px)': 3,
  16:17:     default: 2,
  17:18: };

The following could also be an option if we only display the new line numbers:

diff --git a/src/playground.js b/src/playground.js
index 33f4085..6ae6bbb 100644
--- a/src/playground.js
+++ b/src/playground.js
@@ -5,13 +5,14 @@ const heightQuery = '(min-height: 400px)';
      5:
      6: const widthSwitch = {
      7:     '(min-width: 600px)': 5,
-  8:        '(min-width: 400px)': 4,
+     8:     '(min-width: 400px)': 4.5,
+     9:     '(min-width: 300px)': 4,
     10:     '(min-width: 200px)': 3,
     11:     default: 2,
     12: };
     13: const heightSwitch = {
     14:     '(min-height: 600px)': 5,
- 14:        '(min-height: 400px)': 4,
+    15:     '(min-height: 300px)': 4,
     16:     '(min-height: 200px)': 3,
     17:     default: 2,
     18: };
mookid commented 4 years ago

44