A better handler for ./coreutils date -f aze
#4482
Conversation
./coreutils date -f aze
src/uu/date/src/date.rs
Outdated
| } | ||
| Err(_) => { | ||
| eprintln!("Error: File not found at path {:?}", path); | ||
| Box::new(std::iter::empty()) |
There was a problem hiding this comment.
could you please add a test to make sure we don't regress ?
There was a problem hiding this comment.
Sure, the tests should go in test_date.rs right? Can't see a test for file stuff on there.
src/uu/date/src/date.rs
Outdated
| Box::new(iter) | ||
| } | ||
| Err(_) => { | ||
| eprintln!("Error: File not found at path {:?}", path); |
There was a problem hiding this comment.
please use the same wording as GNU
$ LANG=C date -f file-non-existing
date: file-non-existing: No such file or directory
There was a problem hiding this comment.
Also, I think the syntax should be like:
coreutils/src/uu/sum/src/sum.rs
Lines 88 to 93 in a75cfd9
and the error code should be 1
There was a problem hiding this comment.
this isn't resolved ;) see my second comment (using USimpleError)
There was a problem hiding this comment.
Aye! Sorry!
Would this work? I'm sorry for bugging you, just new to GNU contributions!
Err(err) => {return Err(USimpleError::new(2, format!("failed to open {}: {}", path.display(), err),));}There was a problem hiding this comment.
please try
but the error message isn't the one I asked you to update ;)
|
and please fix the clippy warning |
|
GNU testsuite comparison: |
|
Error code now: |
|
GNU testsuite comparison: |
tests/by-util/test_date.rs
Outdated
| .fails(); | ||
| result.no_stdout(); | ||
| // should fail | ||
| assert!(result.stderr_str().starts_with("date: ")); |
There was a problem hiding this comment.
please test the error message too here
src/uu/date/src/date.rs
Outdated
| @@ -204,7 +204,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { | |||
| return set_system_datetime(date); | |||
| } else { | |||
| // Declare a file here because it needs to outlive the `dates` iterator. | |||
| let file: File; | |||
| let _file: File; | |||
There was a problem hiding this comment.
To keep clippy happy, seems it was only being used for the previous implementation, should I keep it as is?
There was a problem hiding this comment.
Surprisingly, it's even unnecessary in the old code :) Seems like the original implementor just assumed that it was necessary even though the BufReader::new and all the other functions take owned values to file isn't dropped anyway. So yeah, let's remove it.
There was a problem hiding this comment.
why you didn't remove it ? :)
There was a problem hiding this comment.
Oh, no particular reason. Let me remove it then.
src/uu/date/src/date.rs
Outdated
| let lines = BufReader::new(file).lines(); | ||
| let iter = lines.filter_map(Result::ok).map(parse_date); | ||
| Box::new(iter) | ||
| match File::open(path) { |
| @@ -198,6 +198,27 @@ fn test_date_set_valid_2() { | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
rustfmt isn't happy either
|
Fixed fmt warnings. Anything else I missed? |
|
thanks :) |
./target/release/coreutils date -f aze doesn't result in panic anymore, instead gives Error: File not found at path . Fixes #4480