Skip to content

Conversation

@Qelxiros
Copy link
Contributor

@Qelxiros Qelxiros commented Apr 6, 2025

closes #7669

sleep now uses uucore's from_str instead of fundu to support hex parsing. This changes some error output (although it looks to be more in line with GNU, not less), so I changed the relevant tests as well. However, at first glance, it looks like it might be returning "empty string" in unexpected cases (e.g. '\n').

@Qelxiros Qelxiros force-pushed the 7669-sleep-hex-parsing branch from fdcaee4 to c42569e Compare April 6, 2025 09:15
@drinkcat
Copy link
Collaborator

drinkcat commented Apr 6, 2025

@Qelxiros Please rebase, #7648 made changes in this code recently.

@Qelxiros Qelxiros force-pushed the 7669-sleep-hex-parsing branch from c42569e to c05b05d Compare April 6, 2025 11:25
@github-actions
Copy link

github-actions bot commented Apr 6, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@Qelxiros Qelxiros force-pushed the 7669-sleep-hex-parsing branch from c6ec491 to 471e100 Compare April 6, 2025 13:18
@github-actions
Copy link

github-actions bot commented Apr 6, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@github-actions
Copy link

github-actions bot commented Apr 6, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre requested a review from Copilot April 6, 2025 22:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

tests/by-util/test_sleep.rs:243

  • The test function name suggests no error is expected, but the test asserts failure by calling .fails(). Consider renaming the test (for example, to test_sleep_when_input_has_trailing_whitespace_then_error) to more accurately reflect its behavior.
fn test_sleep_when_input_has_trailing_whitespace_then_no_error(#[case] input: &str) {

@github-actions
Copy link

github-actions bot commented Apr 7, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

Copy link
Collaborator

@drinkcat drinkcat left a comment

Choose a reason for hiding this comment

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

Thank you!! I think it looks quite good (I was afraid it'd look worse than that ,-P)! Just a few nits.

@drinkcat
Copy link
Collaborator

drinkcat commented Apr 7, 2025

Oh, one more thing, please squash your commits into logical units (1 big commit would be ok, or maybe a couple: one for num_parser and one for parse_time), so that the git history stays somewhat clean.

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Apr 8, 2025

@drinkcat There should be an option to "squash and merge", which will combine all of my changes into one commit (possibly two with a merge commit). If it's easier for you, I'm happy to make one commit on my end.

@Qelxiros Qelxiros force-pushed the 7669-sleep-hex-parsing branch from 9e807b4 to 5033605 Compare April 8, 2025 01:10
@github-actions
Copy link

github-actions bot commented Apr 8, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

@drinkcat
Copy link
Collaborator

drinkcat commented Apr 8, 2025

Thanks! I think this looks good now.

@drinkcat There should be an option to "squash and merge", which will combine all of my changes into one commit (possibly two with a merge commit). If it's easier for you, I'm happy to make one commit on my end.

I'll let @sylvestre or @cakebaker comment.

@Qelxiros
Copy link
Contributor Author

I've confirmed that this would close #7678, but the relevant tests aren't there yet. This implementation of from_str also has the potential to close #7670.

However, I don't want this PR to get too much scope creep. Should I add these two features here, or should I wait for this PR to get merged and then branch off of main for the other two?

@github-actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@RenjiSann
Copy link
Collaborator

There should be an option to "squash and merge", which will combine all of my changes into one commit (possibly two with a merge commit). If it's easier for you, I'm happy to make one commit on my end.

As a good practice, I usually make 2 commits (unless I'm pushing a massive refactor which needs separate commits) : One for the changes, one for the tests. This is not possible to do using Github's interface, hence the request to squash the commits on your side.

Thank you !

@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/misc/sleep is no longer failing!

@sylvestre
Copy link
Contributor

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

kudos :)

@Qelxiros Qelxiros force-pushed the 7669-sleep-hex-parsing branch from b80dd49 to 7f2fb04 Compare April 14, 2025 11:35
@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/misc/sleep is no longer failing!

@RenjiSann RenjiSann merged commit 3dcee17 into uutils:main Apr 14, 2025
69 checks passed
@RenjiSann
Copy link
Collaborator

Thanks !

@sylvestre
Copy link
Contributor

@Qelxiros as you touched this, maybe you could also remove the fundu dependency here too:

// Advantage of `fundu` over `Duration::(try_)from_secs_f64(source.parse().unwrap())`:

drinkcat added a commit to drinkcat/coreutils that referenced this pull request Apr 15, 2025
@Qelxiros Qelxiros deleted the 7669-sleep-hex-parsing branch April 18, 2025 04:56
nickorlow pushed a commit to nickorlow/coreutils that referenced this pull request Jul 17, 2025
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.

sleep: Duration parsing does not support all float formats (e.g. hex)

4 participants