-
Notifications
You must be signed in to change notification settings - Fork 379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Blame line numbers #885
Blame line numbers #885
Conversation
src/cli.rs
Outdated
/// on every line; "{block}" to only show the line number when a new blame block starts; or | ||
/// "{every-N}" to show the line number with every block and every N-th (modulo) line. | ||
#[structopt(long = "blame-line-number-suffix", default_value = "│{:^4}│")] | ||
pub blame_line_number_suffix: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about how to increase consistency with other options (I think that with so many options, consistency is valuable if we are to retain some sanity). How about
- Call this
blame_separator_format
. All the other options that have{placeholder}
s have a name like*-format
. - Support
{n}
to mean: replace this placeholder with the current line number. This gives consistency withline-numbers-{left,right}-format
, sort of -- those are{nm}
and{np}
to distinguish the minus and plus line numbers. - Is it natural that empty placeholder
{}
means insert line number, or should we not support that? - Now, the way I'm thinking of it,
{block}
and{every-N}
are "magical" in that they are not placeholders in the sense that it's used elsewhere, but rather directives saying when line numbers should be inserted. I'm wondering whether{n:block}
and{n:every-N}
are even more justifiable, meaning "insert line numbers, but subject to a certain policy".
I note that hyperlinks-file-link-format
uses {line}
to mean current line number. Possibly that should be changed to {n}
, although the breaking change is probably not worth it. Maybe at least that one should have {n}
support added and maybe {line}
dropped from docs or mentioned as deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also
- Do you think we need the
off
special case? It's the same as│
I believe. Perhaps, users who care are going to be just as capable of looking up the default and copying the│
character, as they are of learning theoff
special case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, now using {n}
, {n:block}
and {n:every-N}
and extended the format regex to support it. Still need to work around StructOpt
displaying {n}
as a newline. An empty {}
could also be supported, but that would make the help text more complicated.
And I think just writing off
is easier than copy/pasting a unicode │
, plus it prevents mistaking it for a plain ascii |
pipe and wondering why the output doesn't look as good as with line numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think just writing
off
is easier than copy/pasting a unicode│
, plus it prevents mistaking it for a plain ascii|
pipe and wondering why the output doesn't look as good as with line numbers.
OK, I'm convinced. What do you think about using none
instead of off
? That would be consistent with --syntax-theme=none
.
This is great, thanks for doing it. I realised after doing the first version of blame support that line numbers were a bit of a glaring omission in the blame output but hadn't done anything about it. |
b60ae7b
to
a5c256f
Compare
Line numbers are now disabled with The type format specifier can now start with a |
Great, let's get this merged. I've rebased it against master: #916 However, I'm seeing an exciting error locally when trying to run the tests on this branch (whether the rebased version or not):
I also note that you have disabled CI on this branch which feels...non-coincidental? :) Are you also seeing that error or is it something about my local environment? I did do |
a5c256f
to
b5cc7bb
Compare
I've rebased this branch, and also reverted the change to the GitHub actions CI config. Just in case I made a mistake in rebasing, the original branch commits are in branch blame_line_numbers_before_rebase. I have no idea yet about my rust compiler crash, but that appears to occur in my local development environment only. There is one test failing on the 32 bit machines:
|
I disabled the tests to not waste compute time since the "Center Align numbers right-ish" commit breaks just about everything involving line numbers. Is the new center-right alignment for numbers Ok with you? If so I can get to fixing these and add actual asserts to the new ones. I noticed the compiler crash as well, seem to be due to incremental compilation again so it does not affect release. I can't reproduce it though, but it was already reported. The fix is to just remove the debug objects: |
Oh, I'm sorry to have held that up. Yes, I agree the "center-aligned" integers look better rightish. |
Similar to Pythons `{n:2.1f}` where f indicates a floating point type. The type may be separated by an underscore: `{n:<15.14_type}`. Add a FormatStringPlaceholderDataAnyPlaceholder template which works without a borrowed Placeholder.
b5cc7bb
to
199288a
Compare
Prefix and suffix of the format string are separator-style highlighted, format options are none, {n}, {n:block}, {n:every-N}.
560b9e7
to
3def7ef
Compare
Ok, push one more commit to re-enable |
Excellent, merged. Fixes #928 |
Adds
--blame-line-number-suffix "off|{}|{block}|{every-N}"
, (and, technically""
):I really want my blame line numbers (and I want them center-right aligned :) - though the usefulness of
every-N
is debatable I admit.