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

unexpand: should allow multiple files #5852 and unexpand: show error message if a directory is specified #5845 #5864

Merged
merged 4 commits into from Jan 24, 2024

Conversation

biplab5464
Copy link
Contributor

@biplab5464 biplab5464 commented Jan 19, 2024

fix in this PR

  • allow multiple files
  • show error if it is directory and continue
  • show error if file doesn't exist and continue

Copy link

GNU testsuite comparison:

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

@biplab5464
Copy link
Contributor Author

do i have to write the test cases also for the above fix ?

@cakebaker
Copy link
Contributor

Yes, please :)

@biplab5464
Copy link
Contributor Author

#[test]
fn test_multiple_files() {
    let (at, mut ucmd) = at_and_ucmd!();

    at.touch("file");
    at.touch("file1");

    ucmd.args(&["file1", "file1"])
        .succeeds()
        .stdout_contains("contents");
}

how to add content to the files
like

echo "contents" > file

@cakebaker
Copy link
Contributor

You can use at.write("file", "content") instead of at.touch().

@biplab5464
Copy link
Contributor Author

why this test is failing Style/spelling (ubuntu-latest, feat_os_unix)

@cakebaker
Copy link
Contributor

It fails because "contenta" isn't a valid English word. I think you are missing a \n after "content" in your test_multiple_files() function:

.stdout_is("content\na        b");

@biplab5464
Copy link
Contributor Author

running 1 test
write(default): /tmp/.tmpbmFGWl/file
write(default): /tmp/.tmpbmFGWl/file1
run: /home/biplab/project/coreutils/target/debug/coreutils unexpand file file1
thread 'test_unexpand::test_multiple_files' panicked at tests/by-util/test_unexpand.rs:259:10:
assertion failed: `(left == right)`

Diff < left / right > :
<contenta        b
>content
>a        b

the test is failing if \n is added the stdout is printing "contenta b" this only

@cakebaker
Copy link
Contributor

Yes, you also have to adapt your implementation ;-)

Best to look at what GNU unexpand does:

$ echo "content" > file
$ echo "a   b" > file2
$ unexpand file file2
content
a   b

Here you can see, that GNU unexpand outputs "a b" on a new line.

Hope this helps.

@biplab5464
Copy link
Contributor Author

biplab5464 commented Jan 22, 2024

biplab@BIPLAB:~/project/coreutils/src/uu/unexpand/src$ cargo run -- file file1
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
     Running `/home/biplab/project/coreutils/target/debug/unexpand file file1`
content
a        b
biplab@BIPLAB:~/project/coreutils/src/uu/unexpand/src$ unexpand file file1
content
a        b
biplab@BIPLAB:~/project/coreutils/src/uu/unexpand/src$ 

here the output is same to GNU i mean in the above it is printing on new line

in the test cases i don't why it is not adding \n for a new line

@cakebaker
Copy link
Contributor

You are right, the implementation works fine :) I don't know yet why there is no \n in the test...

@biplab5464
Copy link
Contributor Author

what should be next step
should i add extra space before "a b"
like " a b"
Style/spelling (ubuntu-latest, feat_os_unix)

this test case will not fail after that
or
it is alright

@cakebaker
Copy link
Contributor

Ok, I figured out I was wrong and your test is correct. Sorry about that.

To make the Style/Spelling test pass, you can add

// spell-checker:ignore contenta

below the license info in test_unexpand.rs.

Comment on lines 264 to 266
let (at, mut ucmd) = at_and_ucmd!();

ucmd.arg("asdf.txt")
Copy link
Contributor

Choose a reason for hiding this comment

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

The at causes an "unused variable"-warning. Instead of at_and_ucmd!() you can simply use new_cmd!() in this case:

Suggested change
let (at, mut ucmd) = at_and_ucmd!();
ucmd.arg("asdf.txt")
new_cmd!()
.arg("asdf.txt")

if filename.is_dir() {
Err(Box::new(USimpleError {
code: 1,
message: format!("unexpand: {}: Is a directory", filename.display()),
Copy link
Contributor

Choose a reason for hiding this comment

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

USimpleError automatically adds the utils name so you don't have to add it manually:

Suggested change
message: format!("unexpand: {}: Is a directory", filename.display()),
message: format!("{}: Is a directory", filename.display()),

@biplab5464
Copy link
Contributor Author

Thanks you for the suggestion

@biplab5464
Copy link
Contributor Author

is my PR ready for merge ?

@cakebaker cakebaker merged commit 150b287 into uutils:main Jan 24, 2024
56 of 61 checks passed
@cakebaker
Copy link
Contributor

And merged, thanks for your PR :)

@biplab5464
Copy link
Contributor Author

Thank you for your cooperation
i learned a lot again

@biplab5464 biplab5464 deleted the test branch January 24, 2024 16:23
@cakebaker
Copy link
Contributor

Great to hear, you are welcome :)

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.

unexpand: should allow multiple files unexpand: show error message if a directory is specified
2 participants