uniq: added support for deprecated `-N` and `+N` options

[Leviticoh]

as asked in issue #2617

i couldn't find a way to get clap to parse them, so at the moment i remove them from the command line before feeding them into clap,

what do you think?

[github-actions[bot]]

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

[Leviticoh]

that's strange, i didn't touch tail and that test passes on my machine

[sylvestre]

it is an intermittent issue in the CI

[sylvestre]

clippy isn't super happy with your change ;)

[Leviticoh]

i hope this time it works, sorry for wasting time, i'm not really experienced with rust

[sylvestre]

No worries, we can tell, your code looks like a way a C++ would write it ;)

[Leviticoh]

i imagine, that's what i'm studying in uni.

currently i only read from the rust book, do you happen to know other good resources to learn rust?

[tertsdiepraam]

currently i only read from the rust book, do you happen to know other good resources to learn rust?

Specifically for this PR, I'd suggest reading the docs for the string methods in Rust, so you don't have to do s.chars().collect(). For example:

• str::start_with
• char::is_numeric

In Rust it's also more common to work with iterators instead of indexing, so looking into that might help too.

If you're looking for more Rust resources in general, there's the blog fasterthanli.me and the YouTube channel by Jon Gjengset, which I both really like.

[tertsdiepraam]

The parsing you've implemented also leads to some weird errors with long arguments that expect values:

❯ cargo run --no-default-features --features=uniq -- uniq --check-chars -10                                                                                                                                 17:04:59
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s
     Running `target/debug/coreutils uniq --check-chars -10`
error: The argument '--check-chars <N>' requires a value but none was supplied

For more information try '--help'

[Leviticoh]

thanks for taking the time to review my code again

In general, I feel like we can make a compromise with support here, by only supporting these numbers when they appear without any other options in the same argument.

this would make the solution much simpler and allow me to remove most of the uglier code, also i'm sure anyone that's unlucky enough to rely on undocumented behaviour isn't going to switch to different implementations anytime soon

The scan and mutable bool in remove_digits still feels a bit un-rust-like and is also wrong if I'm not mistaken, because uniq allows the argument to appear multiple times.

it's ugly, but it was needed to detect if a digit was a flag or the value given to one, for example using remove_digits on -34s7 would give -s7 as an answer because once s is reached found is set to true and digits stop being removed from that argument, and this matches the behaviour in the GNU implementation, still i look forward to drop it

Unrelated, but I find GNU's implementation super unintuitive. Like who thought that these two being identical was a good idea?

if i had to guess, probably noone, i think that behaviour is an unwanted consequence of how they abused getopt to get the values for that option

The parsing you've implemented also leads to some weird errors with long arguments that expect values

that is a problem, luckily uniq doesn't accept negative values in any option as far as i know, but it would still be a confusing message

[sylvestre]

@Leviticoh sorry, it is now conflicting, could you please have a look ?

[Leviticoh]

hi, sorry for the silence, i had some exams to take and couldn't work on this until now,

i couldn't figure out a way to reliably filter the options, so i started from scratch and found out that you can actally do something with clap, at the moment only the option for fields is implemented

[sylvestre]

no worries, we are all volunteers here (afaik) :)

I hope you did great at your exams

[Leviticoh]

fixed a bit the -N option, now it works like in fold

[Leviticoh]

touched it up a bit and i think it's ready

gonna change the title of the pr to reflect that the +N option isn't implemented

[Leviticoh]

sorry, forgot to rebase

[sylvestre]

@Leviticoh @tertsdiepraam should we land it as it? thanks

[Leviticoh]

i think so, sorry for being inactive the last couple of months, school got in the way again, i'd just like to check it again tomorrow, after the last exam of this session, to make sure i didn't forget anything stupid

[Leviticoh]

@sylvestre i read it again and think it's ready @tertsdiepraam i addressed the changes you asked, is it good to go?

[sylvestre]

Thanks :) sorry it took that long :(