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.

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

fmt: fix panic on width argument #5159

Merged
merged 2 commits into from Aug 15, 2023
Merged

fmt: fix panic on width argument #5159

merged 2 commits into from Aug 15, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 14, 2023

fix #5150

Changed the confirmation method from get_flag to contains_id. And added several tests.

Copy link
Contributor

@cakebaker cakebaker left a comment

Choose a reason for hiding this comment

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

Great to see some fmt tests :)

Comment on lines 47 to 55
#[test]
fn test_fmt_goal() {
for param in ["-g", "--goal"] {
new_ucmd!()
.args(&["one-word-per-line.txt", param, "7"])
.succeeds()
.stdout_is("this is\na file\nwith one\nword per\nline\n");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The expected output of this test is different than the output of GNU fmt. When I run GNU fmt with fmt tests/fixtures/fmt/one-word-per-line.txt -g7 I get the following output:

this is a
file with one
word per line

So I would either mark this test with #[ignore] and/or add a comment with a todo.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for reviewing. I fixed test and ignore it.

- fix test to get same result as GNU fmt
@ghost ghost requested a review from cakebaker August 14, 2023 16:50
@cakebaker cakebaker merged commit 7fcff30 into uutils:main Aug 15, 2023
@cakebaker
Copy link
Contributor

Thanks for your PR!

@ghost ghost deleted the fix-fmt-check-opt_width branch August 15, 2023 06:22
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.

fmt panic on width argument
1 participant