JuliaIO / JLD2.jl

HDF5-compatible file format in pure Julia
Other
547 stars 85 forks source link

Do not access .size field in DataType #438

Closed eschnett closed 1 year ago

eschnett commented 1 year ago

Closes https://github.com/JuliaIO/JLD2.jl/issues/428.

codecov[bot] commented 1 year ago

Codecov Report

Base: 84.09% // Head: 84.07% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (904a3e6) compared to base (cdf2ab8). Patch coverage: 88.88% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #438 +/- ## ========================================== - Coverage 84.09% 84.07% -0.03% ========================================== Files 30 30 Lines 4113 4119 +6 ========================================== + Hits 3459 3463 +4 - Misses 654 656 +2 ``` | [Impacted Files](https://codecov.io/gh/JuliaIO/JLD2.jl/pull/438?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIO) | Coverage Δ | | |---|---|---| | [src/data/type\_defs.jl](https://codecov.io/gh/JuliaIO/JLD2.jl/pull/438/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIO#diff-c3JjL2RhdGEvdHlwZV9kZWZzLmps) | `88.88% <71.42%> (-11.12%)` | :arrow_down: | | [src/data/number\_types.jl](https://codecov.io/gh/JuliaIO/JLD2.jl/pull/438/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIO#diff-c3JjL2RhdGEvbnVtYmVyX3R5cGVzLmps) | `73.68% <100.00%> (ø)` | | | [src/data/reconstructing\_datatypes.jl](https://codecov.io/gh/JuliaIO/JLD2.jl/pull/438/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIO#diff-c3JjL2RhdGEvcmVjb25zdHJ1Y3RpbmdfZGF0YXR5cGVzLmps) | `76.43% <100.00%> (ø)` | | | [src/data/writing\_datatypes.jl](https://codecov.io/gh/JuliaIO/JLD2.jl/pull/438/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIO#diff-c3JjL2RhdGEvd3JpdGluZ19kYXRhdHlwZXMuamw=) | `96.84% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIO). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaIO)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

eschnett commented 1 year ago

The failing check is a spurious small apparent reduction in coverage because codecov miscounts some function metadata. There is no actual reduction in coverage.

JonasIsensee commented 1 year ago

Hi @eschnett, thanks a lot for figuring this out!

IanButterworth commented 10 months ago

This issue resurfaced because julia compat was not updated https://github.com/JuliaRegistries/General/pull/93505

giordano commented 10 months ago

It'd be appreciated if package developers proactively added appropriate compat bounds in the registry to avoid troubles to users, when they fix errors like this one.

JonasIsensee commented 10 months ago

Hi @giordano ,

I'm aware of the problem but don't have a proper solution.

Can I limit the compat on Julia (by default on the current minor version) without preventing tests on nightly?

Should I just manually keep track of problems and make handcrafted pull requests to the registry?

giordano commented 10 months ago

We already fixed it in the PR linked above: https://github.com/JuliaRegistries/General/pull/93505. That was more a suggestion for the future.

JonasIsensee commented 10 months ago

Sure, I was just wondering whether I can prevent his from happening again or whether the best/only option is for me to create such a PR next time.

The current JLD2 release claims to be compatible with all future 1.x releases which is probably not correct. (Since it relies on Julia internals and I don't think it's possible to get rid of this entirely)

giordano commented 10 months ago

Ideally you wouldn't use julia's internals. If that's really not possible, when you become aware of some breakage, like #428, it'd be good to do a PR to General like the one linked above, to prevent the package to be installed on incompatible versions of julia.