Skip to content
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

nl: implement -d/--section-delimiter #5158

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

cakebaker
Copy link
Contributor

This PR implements -d/--section-delimiter that allows you to define a section delimiter other than the default \:. If the specified delimiter is a single character, : is added to the delimiter.

Comment on lines 136 to 145
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,
}
Copy link
Member

@tertsdiepraam tertsdiepraam Aug 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 132 to 135
if self.delimiter.is_empty() {
return SectionDelimiter::None;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does making self.delimiter an Option<SectionDelimiter> make sense? Or maybe the whole SectionDelimiterParser?


impl SectionDelimiterParser {
fn new(delimiter: &str) -> Self {
let delimiter = if delimiter.chars().count() == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let delimiter = if delimiter.chars().count() == 1 {
let delimiter = if delimiter.len() == 1 {

Copy link
Contributor Author

@cakebaker cakebaker Aug 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh I see. Maybe it should even be unicode width?

@cakebaker
Copy link
Contributor Author

@tertsdiepraam I addressed your feedback in the latest push in the following way:

  • I replaced chars().count() with len() and added an additional test case test_non_ascii_one_char_section_delimiter
  • I rewrote the "weird" match (plus moved the entire function to SectionDelimiter)
  • the parse function now returns an Option<SectionDelimiter>

In addition I got rid of the SectionDelimiterParser.

Footer,
}

impl SectionDelimiter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is beautiful now!

@tertsdiepraam
Copy link
Member

CI failures seem unrelated, but I'm rerunning them just to be sure.

@cakebaker cakebaker merged commit 035032c into uutils:main Sep 29, 2023
44 of 45 checks passed
@cakebaker
Copy link
Contributor Author

Somehow I overlooked that this PR got approved a while ago :|

@cakebaker cakebaker deleted the nl_section_delimiter branch September 29, 2023 08:43
@uutils uutils deleted a comment from github-actions bot Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants