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

Specify file format explicitly #119

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

devmotion
Copy link
Member

This PR fixes #117 - and does not seem to break the tests. It specifies the file format explicitly when loading ".rda" files instead of relying on the automatic discovery in FileIO. This PR is an alternative to #118.

@timholy My guess is that FileIO 1.6 changed the query mechanism. It seems it prioritized the file ending in FileIO <= 1.5 but with FileIO 1.6 the file type is determined incorrectly as Gzip due to the magic bytes of compressed RData files.

@codecov-io
Copy link

codecov-io commented Mar 6, 2021

Codecov Report

Merging #119 (47b34c2) into master (c2b17d0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #119   +/-   ##
=======================================
  Coverage   84.00%   84.00%           
=======================================
  Files           4        4           
  Lines          25       25           
=======================================
  Hits           21       21           
  Misses          4        4           
Impacted Files Coverage Δ
src/RDatasets.jl 100.00% <ø> (ø)
src/dataset.jl 90.90% <100.00%> (ø)

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 c2b17d0...47b34c2. Read the comment docs.

Copy link

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this, and again sorry for the inconvenience.

If you'd prefer to rely on the query (as a possible safeguard), FileIO 1.6.1 should fix this. But there may be good reason to just skip the query, so I think this might be worth doing. (I don't know this package well enough to offer concrete advice, all I can say is that if you're controlling all the files then it doesn't seem crazy to skip it.)

src/dataset.jl Outdated Show resolved Hide resolved
@devmotion devmotion mentioned this pull request Mar 7, 2021
timholy added a commit to JuliaIO/FileIO.jl that referenced this pull request Mar 8, 2021
timholy added a commit to JuliaIO/FileIO.jl that referenced this pull request Mar 8, 2021
@devmotion
Copy link
Member Author

Bump 😃 What is the general opinion? Should this change be applied, even if FileIO already fixed the original issue?

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 81.48%. Comparing base (b1a5959) to head (fc0eedb).

Files Patch % Lines
src/dataset.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
- Coverage   83.33%   81.48%   -1.86%     
==========================================
  Files           3        4       +1     
  Lines          24       27       +3     
==========================================
+ Hits           20       22       +2     
- Misses          4        5       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

'dataset' suddenly stopped working with 'ISLR'
5 participants