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

Allow UnionAll in Unions #206

Merged
merged 18 commits into from Aug 15, 2020
Merged

Allow UnionAll in Unions #206

merged 18 commits into from Aug 15, 2020

Conversation

JonasIsensee
Copy link
Collaborator

@JonasIsensee JonasIsensee commented Jul 21, 2020

This PR modifies the way Unions are written to file and thus allows saving unions that contain UnionAlls
such as Vector e.g. any parametric struct without the parameterization.

This needs tests!

Important
This is a breaking change.

  • Needs a major version number change
  • Ideally allow for a compat mode. (Change version in file header and then use old machinery to read if old file header is detected ?)

closes #187
closes #109
closes #65

EDIT
This is now backwards compatible in that old files can still be read!

src/data.jl Outdated Show resolved Hide resolved
src/data.jl Outdated Show resolved Hide resolved
@JonasIsensee
Copy link
Collaborator Author

@DilumAluthge ,
This particular bit of code has not been touched in years but it seems that you at least had a look at the DataType reconstruction recently.

Would you be willing to inspect my changes?

@DilumAluthge
Copy link
Member

DilumAluthge commented Jul 25, 2020

Honestly, I don't think I'm qualified to review this PR. It's been a while since I looked at this part of the code, and even then I wasn't very familiar with it.

I looked over the changes and nothing seemed glaringly incorrect, but take that with a huge grain of salt.

@DilumAluthge
Copy link
Member

It looks like this change might be backwards-compatible.

That being said, it is a significant change to existing code. Just to be 100% safe, it might be a good idea to make this PR a breaking change, i.e. bump the version number to 0.2.0.

@JonasIsensee
Copy link
Collaborator Author

Thank you for looking at it.
I fear that I may not find people "qualified" and willing to review PRs for most of the things I want to address in the near future.
With more testing I hope I should be able to get things working anyway.

I definitely agree that this not a patch release since files written with this code will not be readable with older JLD2 versions.

@JonasIsensee JonasIsensee mentioned this pull request Aug 8, 2020
@JonasIsensee JonasIsensee changed the title [WIP] Allow UnionAll in Unions Allow UnionAll in Unions Aug 13, 2020
@codecov
Copy link

codecov bot commented Aug 15, 2020

Codecov Report

Merging #206 into master will increase coverage by 0.16%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   88.86%   89.02%   +0.16%     
==========================================
  Files          19       20       +1     
  Lines        2182     2206      +24     
==========================================
+ Hits         1939     1964      +25     
+ Misses        243      242       -1     
Impacted Files Coverage Δ
src/JLD2.jl 95.55% <ø> (ø)
src/data.jl 81.45% <97.05%> (+0.41%) ⬆️
src/backwards_compatibility.jl 100.00% <100.00%> (ø)
src/file_header.jl 76.92% <0.00%> (+15.38%) ⬆️

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 b2803d4...6ee7216. Read the comment docs.

@JonasIsensee JonasIsensee merged commit 5812641 into master Aug 15, 2020
@JonasIsensee JonasIsensee deleted the unionallinunion branch February 6, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants