A better handler for `./coreutils date -f aze` #4482

AbhinavMir · 2023-03-09T00:08:07Z

./target/release/coreutils date -f aze doesn't result in panic anymore, instead gives Error: File not found at path . Fixes #4480

sylvestre · 2023-03-09T07:30:12Z

src/uu/date/src/date.rs

+ }
+ Err(_) => {
+ eprintln!("Error: File not found at path {:?}", path);
+ Box::new(std::iter::empty())


could you please add a test to make sure we don't regress ?

Sure, the tests should go in test_date.rs right? Can't see a test for file stuff on there.

sylvestre · 2023-03-09T07:31:13Z

src/uu/date/src/date.rs

+ Box::new(iter)
+ }
+ Err(_) => {
+ eprintln!("Error: File not found at path {:?}", path);


please use the same wording as GNU

$ LANG=C date -f file-non-existing date: file-non-existing: No such file or directory

Also, I think the syntax should be like:

coreutils/src/uu/sum/src/sum.rs

Lines 88 to 93 in a75cfd9

if path.metadata().is_err() {

return Err(USimpleError::new(

2,

format!("{}: No such file or directory", name.maybe_quote()),

));

};

and the error code should be 1

My bad! Fixed :)

this isn't resolved ;) see my second comment (using USimpleError)

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),));}

please try
but the error message isn't the one I asked you to update ;)

sylvestre · 2023-03-09T07:58:18Z

and please fix the clippy warning

github-actions · 2023-03-09T08:23:10Z

GNU testsuite comparison:

Congrats! The gnu test tests/misc/tee is no longer failing!

AbhinavMir · 2023-03-09T08:54:35Z

Error code now: date: aze: No such file or directory
Tests: All passed (added one for testing valid and then one for testing invalid files)
Clippy: No warnings

github-actions · 2023-03-09T09:23:18Z

GNU testsuite comparison:

Congrats! The gnu test tests/rm/rm2 is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

sylvestre · 2023-03-09T09:28:56Z

tests/by-util/test_date.rs

+ .fails();
+ result.no_stdout();
+ // should fail
+ assert!(result.stderr_str().starts_with("date: "));


please test the error message too here

sylvestre · 2023-03-09T09:29:05Z

src/uu/date/src/date.rs

@@ -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;


To keep clippy happy, seems it was only being used for the previous implementation, should I keep it as is?

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.

Ah, great! Thx!

why you didn't remove it ? :)

Oh, no particular reason. Let me remove it then.

sylvestre · 2023-03-09T21:59:02Z

src/uu/date/src/date.rs

- let lines = BufReader::new(file).lines();
- let iter = lines.filter_map(Result::ok).map(parse_date);
- Box::new(iter)
+ match File::open(path) {


rustfmt isn't happy :)

sylvestre · 2023-03-09T21:59:11Z

tests/by-util/test_date.rs

@@ -198,6 +198,27 @@ fn test_date_set_valid_2() {
 }
 }

+#[test]


rustfmt isn't happy either

AbhinavMir · 2023-03-09T23:44:58Z

Fixed fmt warnings. Anything else I missed?

sylvestre · 2023-03-18T13:20:15Z

thanks :)

AbhinavMir changed the title ~~Fixes #4480~~ Fixes https:/uutils/coreutils/issues/4480 Mar 9, 2023

AbhinavMir changed the title ~~Fixes https:/uutils/coreutils/issues/4480~~ A better handler for ./coreutils date -f aze Mar 9, 2023

sylvestre reviewed Mar 9, 2023

View reviewed changes

tests/by-util/test_date.rs

@@ -198,6 +198,27 @@ fn test_date_set_valid_2() {

}

}

#[test]

Copy link

Contributor

sylvestre Mar 9, 2023

Choose a reason for hiding this comment

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

rustfmt isn't happy either

sylvestre force-pushed the main branch from 890d974 to 541fe6b Compare March 9, 2023 23:22

AbhinavMir added 7 commits March 12, 2023 11:08

wip

814aec1

GNU-style error

5c4b646

update test_date.rs

27d7dba

Add tests, fix error code

4294a09

wip

226065f

Fixing tests

b02d63d

fmt fixes

60bac4c

sylvestre force-pushed the main branch from 7a56a5f to 60bac4c Compare March 12, 2023 10:08

remove a now useless declaration

cef4c86

sylvestre merged commit 89b83c2 into uutils:main Mar 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A better handler for `./coreutils date -f aze` #4482

A better handler for `./coreutils date -f aze` #4482

AbhinavMir commented Mar 9, 2023 •

edited

Loading

sylvestre Mar 9, 2023

AbhinavMir Mar 9, 2023

sylvestre Mar 9, 2023

sylvestre Mar 9, 2023

sylvestre Mar 9, 2023

AbhinavMir Mar 9, 2023

sylvestre Mar 9, 2023

AbhinavMir Mar 9, 2023 •

edited

Loading

sylvestre Mar 9, 2023

sylvestre commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

AbhinavMir commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

sylvestre Mar 9, 2023

sylvestre Mar 9, 2023

AbhinavMir Mar 9, 2023

tertsdiepraam Mar 9, 2023

AbhinavMir Mar 9, 2023

sylvestre Mar 10, 2023

AbhinavMir Mar 11, 2023

sylvestre Mar 9, 2023

sylvestre Mar 9, 2023

AbhinavMir commented Mar 9, 2023

sylvestre commented Mar 18, 2023

	if path.metadata().is_err() {
	return Err(USimpleError::new(
	2,
	format!("{}: No such file or directory", name.maybe_quote()),
	));
	};

@@ @@ -198,6 +198,27 @@ fn test_date_set_valid_2() { @@
  }
 }
+#[test]

A better handler for ./coreutils date -f aze #4482

A better handler for ./coreutils date -f aze #4482

Conversation

AbhinavMir commented Mar 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhinavMir Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylvestre commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

AbhinavMir commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

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

AbhinavMir commented Mar 9, 2023

sylvestre commented Mar 18, 2023

A better handler for `./coreutils date -f aze` #4482

A better handler for `./coreutils date -f aze` #4482

AbhinavMir commented Mar 9, 2023 •

edited

Loading

AbhinavMir Mar 9, 2023 •

edited

Loading