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

--filter argument for split #1681

Merged
merged 37 commits into from
Jan 18, 2021
Merged

--filter argument for split #1681

merged 37 commits into from
Jan 18, 2021

Conversation

FelipeLema
Copy link
Contributor

Be able to pass the bytes and filename to a child shell process.

  • start a shell process with the provided command-as-a-string
  • forward filename (xaa, xab, xac... ) as $FILE env var
  • send bytes to each shell process

- add --filter as parameter
- carry filter parameter within `Settings`
- add `FilterWriter` to expose the stdin of a shell process
  conveniently as "child process that can be written to" (the process
      dies when the writer dies)
only unix for now... need to re-write for windows (is sed even
available?)
the command needs not to be quoted here. Test was failing because of
failing command, although it should've failed earlier (around
`succeeds()`)
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
tests/by-util/test_split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
@FelipeLema
Copy link
Contributor Author

still missing windows tests and a test with a failed command

@sylvestre
Copy link
Sponsor Contributor

I guess you noticed that Windows is failing

@FelipeLema
Copy link
Contributor Author

FelipeLema commented Jan 15, 2021

well, right now I'm confused on how to write sh -c 'cat > $FILE' in windows-esque

Is it cmd /c print /v "" > "%FILE%" ? (see discussion here) Right now I'm passing it as cmd, /c, print /v "" > "%FILE%"

@FelipeLema
Copy link
Contributor Author

OH MY DAYS

Handling cmd arguments has been a problem for a while

I'll change this to a "non-windows"-only feature, then.

@FelipeLema FelipeLema marked this pull request as ready for review January 16, 2021 15:06
Copy link
Sponsor Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!
Just some minor comments

src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
tests/by-util/test_split.rs Show resolved Hide resolved
tests/by-util/test_split.rs Outdated Show resolved Hide resolved
tests/by-util/test_split.rs Outdated Show resolved Hide resolved
tests/common/util.rs Show resolved Hide resolved
src/uu/split/src/split.rs Outdated Show resolved Hide resolved
src/uu/split/src/split.rs Show resolved Hide resolved
FelipeLema and others added 6 commits January 16, 2021 12:37
- use `show_error!` instead of `crash!`
- cfg-guard code not used in windows builds (remove warnings of unused
code)
It was not my intention to modify or to stage this file
}
}
}
#[cfg(unix)]
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

this is a lot of "cfg(unix)"
what about moving that into a specific platform file?
like
https://github.com/uutils/coreutils/tree/master/src/uu/tail/src/platform/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, no problem

@sylvestre sylvestre merged commit 88911be into uutils:master Jan 18, 2021
@sylvestre
Copy link
Sponsor Contributor

Well done :)

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.

2 participants