Skip to content

numfmt: add --invalid option#4249

Merged
cakebaker merged 24 commits intouutils:mainfrom
sbentmar:numfmt-add-invalid-option
Jul 1, 2023
Merged

numfmt: add --invalid option#4249
cakebaker merged 24 commits intouutils:mainfrom
sbentmar:numfmt-add-invalid-option

Conversation

@sbentmar
Copy link
Contributor

@sbentmar sbentmar commented Dec 31, 2022

Implements the --invalid option from issue #1280.

@sbentmar sbentmar force-pushed the numfmt-add-invalid-option branch from 6429e21 to 6203822 Compare December 31, 2022 02:32
@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
Collaborator

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

tertsdiepraam
tertsdiepraam requested changes Dec 31, 2022
View reviewed changes
Copy link
Collaborator

@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

sbentmar commented Dec 31, 2022

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

@tertsdiepraam
Copy link
Collaborator

tertsdiepraam commented Dec 31, 2022

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 :)

@sbentmar sbentmar force-pushed the numfmt-add-invalid-option branch from 53ca5e7 to b17fda5 Compare January 2, 2023 15:02
tertsdiepraam
tertsdiepraam reviewed Jan 3, 2023
View reviewed changes
Copy link
Collaborator

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

@sbentmar sbentmar force-pushed the numfmt-add-invalid-option branch from e9f42cb to f9c83b3 Compare January 3, 2023 23:32
@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

sbentmar commented Jun 21, 2023

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

@sbentmar sbentmar force-pushed the numfmt-add-invalid-option branch from e26c764 to 969738c Compare June 21, 2023 09:50
@sbentmar sbentmar requested a review from tertsdiepraam June 21, 2023 10:39
@sbentmar
Copy link
Contributor Author

sbentmar commented Jun 21, 2023

@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

github-actions bot commented Jun 23, 2023

GNU testsuite comparison:

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

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

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

cakebaker and others added 2 commits July 1, 2023 15:58
@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

cakebaker commented Jul 1, 2023

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 <sledru@mozilla.com>

* numfmt: remove empty line

---------

Co-authored-by: Daniel Hofstetter <daniel.hofstetter@42dh.com>
Co-authored-by: Sylvestre Ledru <sledru@mozilla.com>
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

@cakebaker cakebaker cakebaker approved these changes

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