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

add support for lz4 decoding #898

Closed
wants to merge 5 commits into from
Closed

add support for lz4 decoding #898

wants to merge 5 commits into from

Conversation

njaard
Copy link

@njaard njaard commented Apr 27, 2018

Requires the lz4 binary which I published in
https://crates.io/crates/lz4util. It has semantics
similar to gzip.

Add a test for lz4 files.

Update the readme

Requires the `lz4` binary which I published in
https://crates.io/crates/lz4util. It has semantics
similar to `gzip`.

Add a test for lz4 files.

Update the readme
@BurntSushi
Copy link
Owner

I'm not sure I follow the choices made in this PR. It seems like lz4 already has a standard command line tool: https://github.com/lz4/lz4 It is even already installed on my system as a core package (Archlinux). If we are going to add support for it, then it should be against the standard binary and not a binary released on crates.io yesterday.

@njaard
Copy link
Author

njaard commented Apr 27, 2018

Since the lz4 command that you cited is compatible with my lz4 (for decompression purposes) due to gzip compatibility, I can simply remove the reference to my crate in the manual. Is that acceptable?

@BurntSushi
Copy link
Owner

@njaard OK, yes, that's fine. It wasn't clear from your PR whether your binary was compatible with the standard one. But yes, might as well remove the crate reference, which I think would be confusing.

Also, please install the standard lz4 binary in CI so that the test for it runs.

README.md Outdated
@@ -106,7 +106,7 @@ increases the times to `2.640s` for ripgrep and `10.277s` for GNU grep.
automatically detecting UTF-16 is provided. Other text encodings must be
specifically specified with the `-E/--encoding` flag.)
* ripgrep supports searching files compressed in a common format (gzip, xz,
lzma or bzip2 current) with the `-z/--search-zip` flag.
lzma, bzip2 or [lz4](https://crates.io/crates/lz4util)) with the `-z/--search-zip` flag.
Copy link
Owner

Choose a reason for hiding this comment

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

Please follow the prevailing format of this file and wrap lines to 79 columns inclusive.

@njaard
Copy link
Author

njaard commented Apr 27, 2018

  • I removed the reference to my crate
  • The line wraps to 79 characters, consequently
  • added liblz4-tool as a dependency for travis
  • found a few more instances to document lz4 support

@@ -1436,7 +1436,7 @@ This flag can be used with the -o/--only-matching flag.
fn flag_search_zip(args: &mut Vec<RGArg>) {
const SHORT: &str = "Search in compressed files.";
const LONG: &str = long!("\
Search in compressed files. Currently gz, bz2, xz, and lzma files are
Search in compressed files. Currently gz, bz2, xz, lzma, and lz4 files are
Copy link

Choose a reason for hiding this comment

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

Can we sort this list alphabetically, just like it is in FAQ.md?

(Sorry for being so OCD!)

Copy link
Author

Choose a reason for hiding this comment

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

I kept the original ordering because it seemed to be by popularity

@njaard
Copy link
Author

njaard commented Jun 8, 2018

@BurntSushi Just a quick reminder that I believe my pull request has satisfied all of your concerns.

@BurntSushi
Copy link
Owner

BurntSushi commented Jul 21, 2018

@njaard Thanks for working on this! I rebased this on top of master, added a CHANGELOG entry and removed your Oxford commas. :-)

@njaard
Copy link
Author

njaard commented Jul 24, 2018

Great!

Also, they were your own Oxford commas :)

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.

3 participants