Skip to content
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.

Sign up for GitHub

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

Jump to bottom

numfmt: add --invalid option #4249

Merged
merged 24 commits into from
Jul 1, 2023
Merged

numfmt: add --invalid option #4249

merged 24 commits into from
Jul 1, 2023

Conversation

sbentmar
Copy link
Contributor

Implements the --invalid option from issue #1280.

@sbentmar sbentmar marked this pull request as draft December 31, 2022 02:33
@sbentmar sbentmar marked this pull request as ready for review December 31, 2022 12:16
@sbentmar sbentmar changed the title [WIP] numfmt: add --invalid option numfmt: add --invalid option Dec 31, 2022
tertsdiepraam
tertsdiepraam reviewed Dec 31, 2022
View reviewed changes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Nice! I think the error handling can be cleaned up a bit using some of the functions in uucore. Could you look into that?

src/uu/numfmt/src/numfmt.rs Outdated Show resolved Hide resolved
src/uu/numfmt/src/numfmt.rs Outdated Show resolved Hide resolved
tertsdiepraam
tertsdiepraam requested changes Dec 31, 2022
View reviewed changes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

See above

@sbentmar
Copy link
Contributor Author

Not sure what's causing these failures, it's all passing locally and the failing components are not numfmt :)

@tertsdiepraam
Copy link
Member

Yeah the GNU tests are a bit flaky. No need to worry about it if it's not concerning the util you changed :)

@sbentmar sbentmar force-pushed the numfmt-add-invalid-option branch 3 times, most recently from 59fddc5 to 53ca5e7 Compare January 2, 2023 00:34
@sbentmar
Copy link
Contributor Author

sbentmar commented Jan 2, 2023

Probably done now, the remaining test failure is not related to this PR :)

tertsdiepraam
tertsdiepraam reviewed Jan 3, 2023
View reviewed changes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Looking good! A few more small comments.

src/uu/numfmt/src/numfmt.rs Outdated Show resolved Hide resolved
src/uu/numfmt/src/options.rs Show resolved Hide resolved
@sbentmar sbentmar force-pushed the numfmt-add-invalid-option branch 2 times, most recently from 9a5f90e to 9664e3e Compare January 28, 2023 22:55
@uutils uutils deleted a comment from github-actions bot Mar 28, 2023
@uutils uutils deleted a comment from github-actions bot Mar 28, 2023
@uutils uutils deleted a comment from github-actions bot Jun 20, 2023
@uutils uutils deleted a comment from github-actions bot Jun 20, 2023
@uutils uutils deleted a comment from github-actions bot Jun 20, 2023
@uutils uutils deleted a comment from github-actions bot Jun 20, 2023
@uutils uutils deleted a comment from github-actions bot Jun 20, 2023
@sbentmar
Copy link
Contributor Author

I'd forgotten about this PR, I'll have a look at what's causing the failures :)

@sbentmar
Copy link
Contributor Author

@sylvestre @tertsdiepraam CI tests are passing again, except for the android thing which doesn't look to be related to this PR :)

The GNU tests related to --invalid are still failing, but they're now failing due to the error message provided not matching instead of the exit code not matching, so that's some kind of improvement. Fixing those error messages (e.g. ! numfmt: invalid suffix in input '4MQ': 'Q' vs ! numfmt: invalid suffix in input: '4MQ') would fix something like 20% of the remaining failures in
numfmt.pl. I might look into those failures soon, but in a separate PR.

@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail-2/inotify-dir-recreate

tertsdiepraam
tertsdiepraam approved these changes Jul 1, 2023
View reviewed changes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I agree this can be merged once Sylvestre's suggestion has been applied!

@github-actions
Copy link

github-actions bot commented Jul 1, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/ls/selinux-segfault is no longer failing!
Skipping an intermittent issue tests/tail-2/inotify-dir-recreate

@cakebaker cakebaker merged commit 42bf580 into uutils:main Jul 1, 2023
@cakebaker
Copy link
Contributor

Thanks for your PR :)

cakebaker added a commit to NikolaiSch/coreutils that referenced this pull request Jul 5, 2023
@sbentmar @cakebaker @sylvestre
numfmt: add --invalid option (uutils#4249)
60df688 
* numfmt: add invalid option

* numfmt: return code 0 if ignore or warn

* numfmt: implement all --invalid modes

* numfmt: validate stdout and stderr

* numfmt: remove unnecessary code

* numfmt: apply formatting

* numfmt: fix clippy issues

* numfmt: fix failing test cases

* numfmt: fix formatting

* numfmt: fix bug when handling broken pipe

* numfmt: fix bug where extra newline was added

* numfmt: add test cases for edge cases

* numfmt: simplify error handling

* numfmt: remove redundant if

* numfmt: add newline between functions

* numfmt: fix failing test cases

* numfmt: add support for arg numbers using --invalid

* numfmt: simplify error handling in value handlers

* numfmt: fix merge conflict and align prints

* numfmt: fix clippy suggestion

* numfmt: replace "valid" with "invalid" in tests

* numfmt: move INVALID to respect alph. order

* numfmt: move printlns outside of match to avoid duplication

Co-authored-by: Sylvestre Ledru <[email protected]>

* numfmt: remove empty line

---------

Co-authored-by: Daniel Hofstetter <[email protected]>
Co-authored-by: Sylvestre Ledru <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre left review comments

@tertsdiepraam tertsdiepraam tertsdiepraam approved these changes

@cakebaker cakebaker cakebaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sbentmar @tertsdiepraam @cakebaker @sylvestre