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

split: implement remaining -n variants and pass GNU tests/split/fail.sh #5252

Merged
merged 41 commits into from
Sep 7, 2023

Conversation

zhitkoff
Copy link
Contributor

@zhitkoff zhitkoff commented Sep 5, 2023

A draft PR to implement 2 missing variants of --number (-n) option for split

K/N
r/K/N

As well as passing GNU tests/split/fail.sh

NOTE: Includes code changes from PR #5222, so this one can be merged instead of #5222

Fixes #5033

zhitkoff and others added 29 commits August 22, 2023 13:09
@zhitkoff
Copy link
Contributor Author

zhitkoff commented Sep 5, 2023

@tertsdiepraam @sylvestre this is rather big one, but all of that had to be included to pass GNU test tests/split/fail.sh
It can replace the PR #5222 for split -1 in as it includes all its functionality (also required to pass that GNU test)
Please let me know how you would like to proceed between these two PRs

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/split/fail is no longer failing!

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/split/fail is no longer failing!

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/split/fail is no longer failing!

@zhitkoff zhitkoff marked this pull request as ready for review September 6, 2023 19:37
.map_err(|_| NumberTypeError::NumberOfChunks(n_str.to_string()))?;
let chunk_number = parse_size(k_str)
.map_err(|_| NumberTypeError::ChunkNumber(k_str.to_string()))?;
if chunk_number > num_chunks || chunk_number == 0 {
Copy link
Sponsor Contributor

@sylvestre sylvestre Sep 6, 2023

Choose a reason for hiding this comment

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

you have the same check 3 times
please using an helper function like:

fn is_invalid_chunk(chunk_number: i32, num_chunks: i32) -> bool {
    chunk_number > num_chunks || chunk_number == 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, will do

@sylvestre
Copy link
Sponsor Contributor

besides these minor two comments, great work :)

@zhitkoff
Copy link
Contributor Author

zhitkoff commented Sep 6, 2023

besides these minor two comments, great work :)

That you! :) I have refactored it a bit following your suggestions

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/split/fail is no longer failing!
GNU test failed: tests/rm/rm2. tests/rm/rm2 is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/split/fail is no longer failing!

@zhitkoff
Copy link
Contributor Author

zhitkoff commented Sep 7, 2023

@sylvestre could you please let me know if you have any other suggestions/comments before this PR can be merged?

@sylvestre
Copy link
Sponsor Contributor

it looks great. @tertsdiepraam don't hesitate if you want to do a post landing review

@sylvestre sylvestre merged commit 80f8eb6 into uutils:main Sep 7, 2023
43 of 45 checks passed
@zhitkoff zhitkoff deleted the split-gnu-test-fail.sh branch November 18, 2023 16:37
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.

split -1 in should be accepted
2 participants