nl: implement -d/--section-delimiter #5158
Conversation
cakebaker
force-pushed
the
nl_section_delimiter
branch
from
2e60c32 to
37cd034
Compare
src/uu/nl/src/nl.rs
Outdated
| match s { | ||
| _ if s == self.delimiter.repeat(3) => SectionDelimiter::Header, | ||
| _ if s == self.delimiter.repeat(2) => SectionDelimiter::Body, | ||
| _ if s == self.delimiter => SectionDelimiter::Footer, | ||
| _ => SectionDelimiter::None, | ||
| } |
There was a problem hiding this comment.
Alright, so this is more of a brainstorm than actual feedback, because this expression is very cool and clever but also a bit weird. Maybe it could be:
if s.trim_left(self.delimiter).is_empty() {
match s.len() / self.delimiter.len() {
3 => SectionDelimiter::Header,
2 => SectionDelimiter::Body,
1 => SectionDelimiter::Footer,
_ => SectionDelimiter::None,
}This avoids the allocations of the strings from repeat as well. But I'm not actually sure it's better.
src/uu/nl/src/nl.rs
Outdated
| if self.delimiter.is_empty() { | ||
| return SectionDelimiter::None; | ||
| } |
There was a problem hiding this comment.
Does making self.delimiter an Option<SectionDelimiter> make sense? Or maybe the whole SectionDelimiterParser?
src/uu/nl/src/nl.rs
Outdated
|
|
||
| impl SectionDelimiterParser { | ||
| fn new(delimiter: &str) -> Self { | ||
| let delimiter = if delimiter.chars().count() == 1 { |
There was a problem hiding this comment.
| let delimiter = if delimiter.chars().count() == 1 { | |
| let delimiter = if delimiter.len() == 1 { |
There was a problem hiding this comment.
Hm, len returns the number of bytes, not the number of chars (for example, "ä".len() returns 2). And my intention was to get the number of chars. On the other hand, using len leads to the same output as GNU nl when specifying ä as delimiter. Which is incorrect, in my opinion ;-) I guess I have to ask in the GNU coreutils project what the expected output should be when using a single non-ASCII char.
There was a problem hiding this comment.
Ohh I see. Maybe it should even be unicode width?
cakebaker
force-pushed
the
nl_section_delimiter
branch
from
37cd034 to
8b9a946
Compare
|
@tertsdiepraam I addressed your feedback in the latest push in the following way:
In addition I got rid of the |
| Footer, | ||
| } | ||
|
|
||
| impl SectionDelimiter { |
|
CI failures seem unrelated, but I'm rerunning them just to be sure. |
cakebaker
force-pushed
the
nl_section_delimiter
branch
from
8b9a946 to
db34255
Compare
|
Somehow I overlooked that this PR got approved a while ago :| |
This PR implements
-d/--section-delimiterthat allows you to define a section delimiter other than the default\:. If the specified delimiter is a single character,:is added to the delimiter.