Add support for fixed width line number display (fixes #544) #723
Conversation
|
In order to handle it as i suggested, i think you would want to have it check the argument string before conversion to But it doesn't really matter either way obv. Would be a cute feature but certainly not important Will add further comments on the diff |
complete/_rg
Outdated
| @@ -45,6 +45,7 @@ _rg() { | |||
| '--ignore-file=[specify additional ignore file]:file:_files' | |||
| '(-v --invert-match)'{-v,--invert-match}'[invert matching]' | |||
| '(-n -N --line-number --no-line-number)'{-n,--line-number}'[show line numbers]' | |||
| '(--line-number-width)'{-l+, --line-number-width=}'[specify width of displayed line number]:number of columns' | |||
There was a problem hiding this comment.
The 'exclusive-or' list at the beginning should exclude all options that this one makes irrelevant. So we would probably want something like this at the very least:
(-l -N --line-number-width --no-line-number)
Then the definition for -N should be updated similarly to exclude --line-number-width
(I think we should also add a few more, like -c, but it seems i haven't accounted for them in the past)
There was a problem hiding this comment.
Oh, i was going to comment that i don't think a short option is necessary, but it seems you reached that conclusion too, since you didn't actually add one. You should remove the -l+ (which is used by something else anyway) then. So it'd be just one string, (...)--line-number-width[...]:...
ETA: And if you remove the -l you don't need to reference --line-number-width in the list any more; it's included by default
There was a problem hiding this comment.
Ah, it is with some embarrassment that I have to admit I was very excited by raising what would be my first PR in an OSS project that I failed to get this part of the change absolutely right. 😞 I'm sorry about that. Thanks for clarifying. I will make the changes you suggested.
And yes, I also felt like we didn't need a short option for this. I briefly considered using -w but decided against it.
There was a problem hiding this comment.
This looks great to me! Nice PR. :-)
My main concern here is supporting arguments like 06 to mean "left pad, using zeroes." Right now, in this PR, I believe 06 will be parsed as 6, which we will left pad with spaces. I am fine with starting there, but I very much expect someone to come along and request zero padding as well. At that point, if we change 06 to left pad zeroes, then it would technically be a breaking change, which I'd like to avoid.
As a simple work-around to this, I'd suggest return an error if the number starts with a 0.
If and when we add a custom format character, we shouldn't use some other library to do left padding for us. Re-implementing this specific part of the format! macro should be a very short helper function.
|
@BurntSushi, I understand your concern. When I was making the change, I thought we wanted it to be very general and support any padding character, which is why I looked into the However, if it is always going to be a choice between 0 or space, I could do what @okdana suggested. Then we can fail if the first character is anything other than a 0. Let me know if this works for you and I can update the PR accordingly. |
|
I would suggest just starting with supporting spaces. We can add more support as needed, but we will need to return an error if the padding width starts with a zero. In fact, we should return an error if the width starts with anything other than 1-9, but that is already true in this PR since it would fail to parse as an integer. |
|
@BurntSushi, Okay, I understand. Will update the PR accordingly. Thanks for the comments. |
|
@balajisivaraman This also needs to update the man page, which is done in this file: https://github.com/BurntSushi/ripgrep/blob/162e085b98f8f2c627a92402d2e38dda04fc3e48/doc/rg.1.md --- If you have pandoc on your system, then run the |
|
@BurntSushi, Oops! For some reason, I thought that was something automatically provided by ETA: Do we need to add the issue number in the commit message, or is it enough to add it to the PR only? I see that it ends up creating a lot of noise in the actual issue thread. (Apologies if I did it wrong and it created noise in this issue's thread.) |
| fn validate_line_number_width(s: String) -> Result<(), String> { | ||
| if s.starts_with("0") { | ||
| Err(String::from("Custom padding characters are currently not supported. \ | ||
| Please enter only a numeric value.")) |
There was a problem hiding this comment.
Sorry to drag this out, but can you add a test for this error? There should be other examples of tests in tests/tests.rs that check that ripgrep errors out.
|
Sorry, that is something I should've known better and added when I made that change itself. It is done now. |
|
@balajisivaraman Awesome! Thanks so much. Great work! |
As per the discussion on #544, this is my initial attempt at adding support for fixed width line numbers. Please review and let me know if things can be improved upon.
A few thoughts I had as I worked on this:
format!macro currently has no way of providing a custom padding character as a parameter. You have to provide it directly in the format string itself. (As per this, only the width can be provided as a parameter.)line-number-width, I've mentioned that it has no effect if--no-line-numberis enabled, which is very obvious. Should I handle any other cases specific to this newly introduced argument to ensure nothing breaks?Thanks in advance!