search

estk / log4rs

A highly configurable logging framework for Rust
Apache License 2.0
998 stars 147 forks source link

Add possibility to compress log files with Zstandard #363

Open cristiano-prato opened 6 months ago

cristiano-prato commented 6 months ago

I would like to be able to compress log files with zstandard instead of gzip.

cristiano-prato commented 6 months ago

Hi, thanks for the feedback! I added some documentation, benchmark and proper error checking, let me know if it is ok for you!

cristiano-prato commented 6 months ago

I changed the documentation as suggested

estk commented 6 months ago

@cristiano-prato Thanks so much for the excellent contribution. One question I had was: what happens when a user does not have zstd installed?

Other than that it lgtm

bconn98 commented 6 months ago

Agreed with @estk on that concern. I was including that in my decision process about the windows comment. Possibly a startup check with an error out?

cristiano-prato commented 6 months ago

@cristiano-prato Thanks so much for the excellent contribution. One question I had was: what happens when a user does not have zstd installed?

Other than that it lgtm

If the zstd executable is missing, the test will fail. If you agree, I will change the test in order to use the library to decompress the archive, instead of the executable.

cristiano-prato commented 6 months ago

I changed the unit test to avoid dependencies with zstd cli. This also allows the test to run on windows.

cristiano-prato commented 3 months ago

Hello, is there anything I can do to improve this PR?

bconn98 commented 3 months ago

@cristiano-prato Ill take another look later today, but no I don't think so. I'm waiting to get a group of good PRs before bugging the repo owner to take a look.

bconn98 commented 3 months ago

This looks good to me. However, we need to wait for #366 & its associated PR to close first.