Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Command line option to apply line number style to unchanged lines #219

Merged
merged 16 commits into from
Jun 25, 2020

Conversation

clnoll
Copy link
Contributor

@clnoll clnoll commented Jun 9, 2020

Adds --number-zero-style option that overrides --number-minus-style and --number-plus-style on unchanged lines if --number is set.

This gives the ability to draw attention to line numbers for changed lines by deemphasizing the numbers for unchanged lines, e.g.:

image

@clnoll clnoll force-pushed the line-number-zero-style branch from f9eafbb to 118f472 Compare June 9, 2020 19:34
@dandavison
Copy link
Owner

dandavison commented Jun 10, 2020

Thanks @clnoll! That screenshot is very compelling. I agree with this plan and I think I'd have realized soon that I wanted this also.

The option name --number-zero-style is making me pause and think we should double-check our naming scheme. It seems clear that delta is going to have lots of options, and I'm thinking that the only way to manage that complexity is going to be pedantically consistent naming...

So, let me have a go at justifying to myself what we have:

When it comes to hunk lines, we're using the terms minus, zero, and plus (e.g. --minus-style, --zero-style, etc). The implied definitions seem to be

  • minus: a line that occurred in the old file only
  • zero: a line that occurs in both files
  • plus: a line that occurs in the new file only

Then, from #190, we have --number-minus-* and --number-plus-*. There the definitions seem to be

  • minus: line numbers in the old file (they go on the left)
  • plus: line numbers in the new file (they go on the right)

So, I think I'm happy with all that. Even though the identified sections are horizontal in the first case and vertical in the second, it seems to be consistent with the basic idea that there's a "minus file" (old file) and a "plus file" (new file).

When it comes to line number display, it seems we have 2 dimensions and a maximum of 6 things that could have names and distinct styles:

| X_11 | X_21 | minus line
| X_12 | X_22 | zero line
| X_13 | X_23 | plus line

So it's like we're using --number-minus-* to refer to the left column X_1* and --number-plus-* to refer to the right column, i.e. X_2*.

And finally, in this PR, we want to choose a name for X_12 and X_22...

What do you think? The only thing I've come up with so far is to add two new options

  • --number-minus-zero-style
  • --number-plus-zero-style

That sort of makes sense as representing indexing into a 2D array, with the first index identifying the column? And leaves open the possibility of adding options for the other things with logical names if that were ever desired (--number-minus-minus-style, etc).

Or is there another way of looking at it that yields a simpler set of names?

There are a couple of related thoughts that aren't really the concern of this PR but I'll mention anyway:

  • It might be getting necessary to organize the --help text so that the commonly used options are not lost in the sea of all possible options.

  • We could maybe do with better ways to express the idea of one option defaulting to the value of another option?

@dandavison dandavison force-pushed the line-number-zero-style branch from 118f472 to d4bc5c2 Compare June 11, 2020 03:06
@clnoll clnoll force-pushed the line-number-zero-style branch from d4bc5c2 to 6086a33 Compare June 13, 2020 23:22
@clnoll
Copy link
Contributor Author

clnoll commented Jun 14, 2020

Thanks for the suggestions, I agree re the inconsistency of row vs. column.

I have a slightly different proposal, now that I've played around with the options a bit.

The first version of the numbers options conflates the concept of minus/left and plus/right, because it colors all numbers in a column, even if the row that the number is associated with was removed or added (aka a zero-row).

I think we should alter the behavior of --number-minus-style and --number-plus-style to apply row-wise. That is, only apply the style if the line was actually removed/added. That leaves the obvious gap for the row-based --number-zero-style, which will have behavior from this PR.

We can then add separate options for styling by entire column (--number-left-style and --number-right-style). I think that if the minus/plus styles are also set they should override the column setting for their respective rows.

The last changes I'd propose would be to have the format strings & corresponding styles be left/right column-based (--number-left-format* and --number-right-format* instead of --number-minus-format* and --number-plus-format*), and instead of having the minus/plus line numbers be determined by which column you're in, we can have separate format string placeholders %lm and %lp, so if someone wanted to swap them that would be possible.

@clnoll
Copy link
Contributor Author

clnoll commented Jun 14, 2020

BTW I'd love to make the placeholders %nm and %np but git pretty-formats use %n for new lines, and I don't want to clash with such a generalized placeholder. (I also noticed that if they use a letter in the 1-letter placeholder, they don't use that letter in any the 2-letter placeholders, and it seems worthwhile to stay consistent with their conventions in case they become useful in other areas).

That being said, let me know if you'd prefer to use something else for the placeholders.

@dandavison
Copy link
Owner

Thanks for thinking about this @clnoll! I think your proposal is better than mine: I agree with everything you wrote in your last two comments.

@dandavison dandavison mentioned this pull request Jun 17, 2020
@clnoll clnoll force-pushed the line-number-zero-style branch 2 times, most recently from eba8c0a to fe5e882 Compare June 21, 2020 05:49
@dandavison dandavison force-pushed the line-number-zero-style branch 2 times, most recently from bf114c1 to 66d7eac Compare June 23, 2020 14:54
@dandavison
Copy link
Owner

dandavison commented Jun 23, 2020

This is looking great. I've pushed a commit that starts adding (high-level) tests for this branch. 3 are passing and one is failing.

I've added the tests in a new file features/numbers.rs. At some point we can move all the number-specific code into that file and make the implementation follow the same pattern as other "features". Incidentally, what do you think about the singular vs plural name? It seems that based on other common command line tools, a flag with the singular name --number should exist. But as a feature, the singular version seems unnatural. Maybe we need to support both as synonyms?

@dandavison dandavison force-pushed the line-number-zero-style branch 3 times, most recently from c0fc8e1 to 43f4282 Compare June 25, 2020 20:15
@dandavison
Copy link
Owner

Thanks again @clnoll! I've added some tests and changed the command line interface as discussed.

    -n, --line-numbers               Display line numbers next to the diff. See LINE NUMBERS section

    --line-numbers-minus-style <line-numbers-minus-style>
        Style (foreground, background, attributes) for line numbers in the old (minus) version of the file. See
        STYLES and LINE NUMBERS sections [default: auto]
    --line-numbers-zero-style <line-numbers-zero-style>
        Style (foreground, background, attributes) for line numbers in unchanged (zero) lines. See STYLES and LINE
        NUMBERS sections [default: auto]
    --line-numbers-plus-style <line-numbers-plus-style>
        Style (foreground, background, attributes) for line numbers in the new (plus) version of the file. See
        STYLES and LINE NUMBERS sections [default: auto]
    --line-numbers-left-format <line-numbers-left-format>
        Format string for the left column of line numbers. A typical value would be "{nm:^4}⋮" which means to
        display the line numbers of the minus file (old version), followed by a dividing character. See the LINE
        NUMBERS section [default: {nm:^4}⋮]
    --line-numbers-right-format <line-numbers-right-format>
        Format string for the right column of line numbers. A typical value would be "{np:^4}│ " which means to
        display the line numbers of the plus file (new version), followed by a dividing character, and a space. See
        the LINE NUMBERS section [default: {np:^4}│ ]
    --line-numbers-left-style <line-numbers-left-style>
        Style (foreground, background, attributes) for the left column of line numbers. See STYLES and LINE NUMBERS
        sections [default: auto]
    --line-numbers-right-style <line-numbers-right-style>
        Style (foreground, background, attributes) for the right column of line numbers. See STYLES and LINE NUMBERS
        sections [default: auto]

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants