-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
…deleted test files that make script fail
@tertsdiepraam @sylvestre this is rather big one, but all of that had to be included to pass GNU test |
GNU testsuite comparison:
|
GNU testsuite comparison:
|
GNU testsuite comparison:
|
src/uu/split/src/split.rs
Outdated
.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 { |
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, will do
besides these minor two comments, great work :) |
That you! :) I have refactored it a bit following your suggestions |
GNU testsuite comparison:
|
GNU testsuite comparison:
|
@sylvestre could you please let me know if you have any other suggestions/comments before this PR can be merged? |
it looks great. @tertsdiepraam don't hesitate if you want to do a post landing review |
A draft PR to implement 2 missing variants of --number (-n) option for
split
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