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

Fix detect_compressed() #318

Merged
merged 3 commits into from
Mar 20, 2021
Merged

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Mar 17, 2021

The PR fixes a typo in BZIP2 magic bytes that is responsible for JuliaData/RData.jl#83 and adds unit tests for all supported compression formats.

I've also refactored the compression detection: now there is detect_compressor() function that returns the compression format code or nothing if no supported format detected.
I would be using it from RData.jl instead of the compression detection code that is currently there to avoid code duplication.
Also, it could help in future when implementing the format detection of decompressed streams.
For example, the current version of FileIO checks for RData-specific magic bytes if the file is not compressed, but for compressed RData files it just checks the compression format but skips magic bytes check of the uncompressed stream (see also #225).

Once merged, it would be nice to tag a new version of FileIO, because the fix of JuliaData/RData.jl#83 depends on it.

@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #318 (26c3a19) into master (3e72cd4) will increase coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
+ Coverage   89.88%   90.22%   +0.33%     
==========================================
  Files          10       10              
  Lines         623      624       +1     
==========================================
+ Hits          560      563       +3     
+ Misses         63       61       -2     
Impacted Files Coverage Δ
src/registry.jl 90.54% <100.00%> (+0.74%) ⬆️
src/precompile.jl 100.00% <0.00%> (+6.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e72cd4...26c3a19. Read the comment docs.

@alyst
Copy link
Contributor Author

alyst commented Mar 19, 2021

@timholy could you please take a look?

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Curious to ask, will it be better to use :GZIP, :BZIP2, :XZ, :LZ4 and perhaps :plain (or :uncompressed) here?

@johnnychen94 johnnychen94 merged commit 94099c8 into JuliaIO:master Mar 20, 2021
@johnnychen94
Copy link
Member

Once merged, it would be nice to tag a new version of FileIO, because the fix of JuliaData/RData.jl#83 depends on it.

FileIO v1.6.5 is out.

@alyst alyst deleted the fix_detect_compressed branch March 20, 2021 09:54
@alyst
Copy link
Contributor Author

alyst commented Mar 20, 2021

Curious to ask, will it be better to use :GZIP, :BZIP2, :XZ, :LZ4 and perhaps :plain (or :uncompressed) here?

Thanks for merging!
I think returning :uncompressed would be misleading as we only check a limited subset of compressors.
I don't have a particular preference to symbols or strings, I've just kept the current choice.

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.

None yet

2 participants