-
-
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: split -1 in
should be accepted
#5222
Conversation
GNU testsuite comparison:
|
@tertsdiepraam here is the new PR for this issue, replacing #5170 |
src/uu/split/src/split.rs
Outdated
@@ -52,14 +52,86 @@ const AFTER_HELP: &str = help_section!("after help", "split.md"); | |||
|
|||
#[uucore::main] | |||
pub fn uumain(args: impl uucore::Args) -> UResult<()> { | |||
let args = args.collect_lossy(); |
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.
I'd like to avoid this where possible. I think we should be operating on OsStr
if we can.
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.
this was a direct copy from how it is handled in fold
, but I will try to see if we can switch to operating on OsStr
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.
Unfortunately I had to keep the same implementation as in fold
, i.e. using uucore::Args
s collect_lossy()
method. There is also collect_ignore()
defined, but I think to stay consistent we should use the same approach as fold
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.
ok, took another pass at it and in the latest commit removed the need for collect_lossy()
and let clap
panic on invalid non UTF-8 arguments + tests to cover that
src/uu/split/src/split.rs
Outdated
let mut obs_lines = None; | ||
let filtered_args = args | ||
.iter() | ||
.filter_map(|slice| { |
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.
This method is still no bulletproof unfortunately. The problem is that something that looks like a short option chunk could be an argument to a long flag. For example:
split --additional-suffix -x400a4
With this PR, this is the output I get:
error: a value is required for '--additional-suffix <SUFFIX>' but none was supplied
For more information, try '--help'.
That's quite confusing...
I think that case is more important to get exactly right than the shortcut.
I think the way that GNU split
does it, is by storing a number and defining short flags for each digit and when that digit in encountered multiply the number by 10 and adding the digit. You can see this too because split -1 -2
behaves like split -12
. I personally think that behaviour is more confusing than helpful, maybe it's the way to go?
You've done a great job with all the case you have taken into account here already, but this method might be a dead end if we want full compatibility. I'd be happy to be proven wrong though!
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 for catching that! I will go deeper into the rabbit hole :)
On the split -1 -2 file
example though - the GNU split would treat it as split -2 file
, i.e. it will not combine -1
with -2
for a total -12
, so at least that part works now :)
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.
It won't? Oh interesting! It's even more clever than I thought then.
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.
updated the code to handle this edge case correctly (in a generic way, should work for all options where it applies) + tests
split --additional-suffix -x400a4
@tertsdiepraam checking in - if there're any other suggestions/modification you have for PR before it can be merged? |
Closing this one as #5252 has been merged instead and it includes all the same code changes as this one |
Handling of obsolete lines option value + tests including possible use cases/combinations with other short options
Fixes #5033