split: better handle numeric and hex suffixes, short and long, with and without values #5198

zhitkoff · 2023-08-24T00:36:19Z

Short variants of numeric and hex suffixes now act as flags with value forced to 0
Long variants work without value (default 0) as well as with value (equal sign is enforced)
Any combination of numeric and/or hex suffixes in any order is allowed with last one winning
Short variants can be combined together like split -dxen 4 file
Tests added to cover changes and verified against GNU split to match behavior

Fixes #5171

…nd without values Fixes uutils#5171

github-actions · 2023-08-24T08:06:50Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail-2/inotify-dir-recreate

tertsdiepraam

Nice! I think this can be cleaned up a bit more by offloading the overriding of the arguments to clap.

tertsdiepraam · 2023-08-24T09:28:30Z

src/uu/split/src/split.rs

                .default_missing_value("0")
                .num_args(0..=1)
+                .overrides_with(OPT_NUMERIC_SUFFIXES)


Each argument should also override with the corresponding option right? Maybe we could also add some tests for that.

yep, tests are actually covering that, but will refactor with .overrides_with_all() instead

tertsdiepraam · 2023-08-24T09:29:37Z

src/uu/split/src/split.rs

+                    Ok("0".to_string()) // force value to "0"
+                })
+                .overrides_with(OPT_NUMERIC_SUFFIXES_SHORT)
+                .help("use numeric suffixes instead of alphabetic"),


Maybe it's nice to say something like "like --numeric-suffixes but does not accept an argument" to explain why there are two arguments that do the same thing.

not sure I understood the suggestion, could you please exand on that?

The suggestion is to put a bit of text in the help string saying that it's the same as --numeric-suffixes. If I wouldn't know about these arguments and I would see two arguments with the same description in --help, I'd be a bit confused. Does that make sense?

yep, that makes sense, thanks. Will update it

Followed exact same order and wording as in GNU split help:
-d use numeric suffixes starting at 0, not alphabetic

--numeric-suffixes[=FROM]
same as -d, but allow setting the start value

-x use hex suffixes starting at 0, not alphabetic

--hex-suffixes[=FROM]
same as -x, but allow setting the start value

tertsdiepraam · 2023-08-24T09:31:00Z

src/uu/split/src/split.rs

+                .value_parser(|_: &str| -> Result<String,clap::Error> {
+                    Ok("0".to_string()) // force value to "0"
+                })


This feels convoluted. We have have to combine the arguments anyway, so it would suffice for this to just be a flag without any value.

well ... it is a flag as far as behaviour goes (and as far as clap works), but it does produce a value. If -d or -x is used , the value must be specifically set to 0 for numeric or hex suffix correspondingly. It can be set here with value_parser() or harcoded later on when suffixes are processed. Would it be better to have all defaults in the same place as arg definitions?

I see where you're coming from. It's an interesting idea to try to make all the flags sort of the same in the code. However, I think I still prefer hardcoding them in the processing of the flags. The thing that I find confusing is that if it is a flag, we should use the functions that are appropriate for flags if that makes sense. value_parser is usually used for parsing user generated values, which we don't have here, so hardcoding the 0 gives us the ability to skip the parsing.

So, for instance, I'd write the match expression to combine the flags something more like this:

match ( matches.get_one::<String>(OPT_NUMERIC_SUFFIXES), matches.get_one::<String>(OPT_HEX_SUFFIXES), matches.get_flag(OPT_NUMERIC_SUFFIXES_SHORT), matches.get_flag(OPT_HEX_SUFFIXES_SHORT), ) { (Some(v), _, _, _) => parse_num_suffix(matches, v), (_, Some(v), _, _) => parse_hex_suffix(matches, v), (_, _, true, _) => Ok((SuffixType::Decimal, 0)), (_, _, _, true) => Ok((SuffixType::Hexadecimal, 0)), _ => Ok((SuffixType::Alphabetic, 0)), // no numeric/hex suffix, using default alphabetic }

(I forgot the exact semantics of those clap functions. Maybe the value source check is necessary as well)

refactored to remove arg .value_parser() and hardcoded default 0 in the fn suffix_type_from()

tertsdiepraam · 2023-08-24T09:33:23Z

src/uu/split/src/split.rs

+    // Compare suffixes by the order specified in command line
+    // last one (highest index) is effective, all others are ignored


It's much easier to make all the arguments override each other :)

yep, refactoring with overrides_with_all() in args definitions

tertsdiepraam

Nice! It's looking pretty nice already. I just have a few more small suggestions/questions.

tertsdiepraam · 2023-08-26T13:58:06Z

src/uu/split/src/split.rs

                .default_missing_value("0")
                .num_args(0..=1)
-                .help("use numeric suffixes instead of alphabetic"),
+                .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT])


I wonder why rustfmt doesn't pick this up? There's probably some string in this builder that makes rustfmt give up on the whole expression.

Yep if you break up this line:

coreutils/src/uu/split/src/split.rs

Line 115 in 4698e8c

"write to shell COMMAND; file name is $FILE (Currently not implemented for Windows)",

into

.help( "write to shell COMMAND; file name is $FILE \ (Currently not implemented for Windows)", ),

rustfmt will kick in.

interesting ... unfortunately for this specific line rustfmt does not do anything regardless how you break it up, so I will format it by hand

Yes, that is indeed what I meant. If you break it up by hand rustfmt will start working again on the other lines.

tertsdiepraam · 2023-08-26T14:00:23Z

src/uu/split/src/split.rs

+        matches.value_source(OPT_NUMERIC_SUFFIXES) == Some(ValueSource::CommandLine),
+        matches.value_source(OPT_NUMERIC_SUFFIXES_SHORT) == Some(ValueSource::CommandLine),
+        matches.value_source(OPT_HEX_SUFFIXES) == Some(ValueSource::CommandLine),
+        matches.value_source(OPT_HEX_SUFFIXES_SHORT) == Some(ValueSource::CommandLine),


Can we avoid value_source? If I recall correctly, we only have to use that if there is some default value, but I don't think these arguments have one.

yeah ... considering they are all set to override each other and no default_value, can refactor

tertsdiepraam · 2023-08-26T14:02:38Z

src/uu/split/src/split.rs

+    // Note: right now, this exact behavior cannot be handled by
+    // `ArgGroup` since `ArgGroup` considers a default value `Arg`
+    // as "defined".


I'm not sure I understand this. What do you mean with that last part?

yep, my bad - copy/pasted from other section to start with and did not remove. will take it out

tertsdiepraam · 2023-08-26T14:05:59Z

tests/by-util/test_split.rs

+#[test]
+fn test_hex_suffix_no_value() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    ucmd.args(&["-n", "4", "--hex-suffixes", "threebytes.txt"])


I think we should test with a larger file here so that we can verify that hex is used (same for the numeric suffix tests).

yep, will do

zhitkoff · 2023-08-26T16:40:28Z

Nice! It's looking pretty nice already. I just have a few more small suggestions/questions.

Thanks! One question : considering that changes for this pull request addressing issue #5171 modify the same two files as the changes for fixing issue #5033 (handling obsolete lines call as in split -1 in) and both dealing with processing arguments - would you prefer that I merge those changes into this PR or have it separate?

tertsdiepraam · 2023-08-26T16:59:59Z

Separate would be best I think. This PR is fairly simple. The other one will be a lot more complicated. So let's get this merged and then move on to the other one.

zhitkoff · 2023-08-28T01:24:21Z

anything else that you think should be included and/or changed to finalize this PR?

tertsdiepraam

Nope! I love it! Thanks!

split: better handle numeric and hex suffixes, short and long, with a…

42df6ac

…nd without values Fixes uutils#5171

tertsdiepraam reviewed Aug 24, 2023

View reviewed changes

zhitkoff and others added 6 commits August 24, 2023 17:22

refactoring with overrides_with_all() in args definitions

fc3bf94

fixed comments

16c8ad8

updated help on suffixes to match GNU

81062e0

comments

d658f26

refactor to remove value_parser()

fb3b0be

Merge branch 'main' into zhitkoff/issue5171

4698e8c

tertsdiepraam reviewed Aug 26, 2023

View reviewed changes

split: refactor suffix processing + updated tests

6005908

split: minor formatting

54dc1d8

tertsdiepraam approved these changes Aug 28, 2023

View reviewed changes

tertsdiepraam merged commit 1eae064 into uutils:main Aug 28, 2023
44 of 45 checks passed

zhitkoff deleted the zhitkoff/issue5171 branch November 18, 2023 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split: better handle numeric and hex suffixes, short and long, with and without values #5198

split: better handle numeric and hex suffixes, short and long, with and without values #5198

zhitkoff commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

tertsdiepraam left a comment

tertsdiepraam Aug 24, 2023

zhitkoff Aug 24, 2023

tertsdiepraam Aug 24, 2023

zhitkoff Aug 24, 2023

tertsdiepraam Aug 24, 2023

zhitkoff Aug 24, 2023

zhitkoff Aug 24, 2023

tertsdiepraam Aug 24, 2023

tertsdiepraam Aug 24, 2023

zhitkoff Aug 24, 2023

tertsdiepraam Aug 24, 2023 •

edited

Loading

zhitkoff Aug 24, 2023

tertsdiepraam Aug 24, 2023

zhitkoff Aug 24, 2023

tertsdiepraam left a comment •

edited

Loading

tertsdiepraam Aug 26, 2023

tertsdiepraam Aug 26, 2023

zhitkoff Aug 26, 2023

tertsdiepraam Aug 26, 2023

tertsdiepraam Aug 26, 2023

zhitkoff Aug 26, 2023

tertsdiepraam Aug 26, 2023

zhitkoff Aug 26, 2023

tertsdiepraam Aug 26, 2023

zhitkoff Aug 26, 2023

zhitkoff commented Aug 26, 2023

tertsdiepraam commented Aug 26, 2023

zhitkoff commented Aug 28, 2023

tertsdiepraam left a comment •

edited

Loading

		// Compare suffixes by the order specified in command line
		// last one (highest index) is effective, all others are ignored

split: better handle numeric and hex suffixes, short and long, with and without values #5198

split: better handle numeric and hex suffixes, short and long, with and without values #5198

Conversation

zhitkoff commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

tertsdiepraam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tertsdiepraam Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tertsdiepraam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhitkoff commented Aug 26, 2023

tertsdiepraam commented Aug 26, 2023

zhitkoff commented Aug 28, 2023

tertsdiepraam left a comment • edited Loading

Choose a reason for hiding this comment

tertsdiepraam Aug 24, 2023 •

edited

Loading

tertsdiepraam left a comment •

edited

Loading

tertsdiepraam left a comment •

edited

Loading