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

Move set_blosc! methods to H5ZBlosc.jl #1167

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

Conversation

topolarity
Copy link

That's the last one gone!

As far as I can tell, Requires was not necessary here since the package it was "extending" already depends on HDF5.jl

As far as I can tell, this usage of Requires was entirely unnecessary
since the package it was "extending" already depends on HDF5.jl

Co-authored-by: Gabriel Baraldi <baraldigabriel@gmail.com>
@topolarity topolarity force-pushed the ct/mv-blosc branch 2 times, most recently from 069d059 to 60d7775 Compare October 9, 2024 23:50
This is needed to pick up Documenter 1.4.0 which supports
fully-qualified `@ref` links.
@mkitti
Copy link
Member

mkitti commented Oct 10, 2024

I need to think about the interaction with #1160. These methods were mainly there was backwards compatibility.

@topolarity
Copy link
Author

For what it's worth, the methods are still there - I think this should behave pretty much as it did before, except that because we can't set retroactive [compat] bounds, it's possible to have, e.g., an old version of H5Zblosc with the new HDF5.jl that will be missing the methods.

@mkitti
Copy link
Member

mkitti commented Oct 12, 2024

@simonbyrne @musm any thoughts about this pull request? Does this require a version bump?

Project.toml Outdated
@@ -22,6 +22,7 @@ MPIExt = "MPI"

[compat]
Compat = "3.1.0, 4"
H5Zblosc = "≥ 0.1.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be covered by the compat in H5Zblosc?

Copy link
Author

Choose a reason for hiding this comment

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

Good point - it should be bumped / tightened whenever this falls into a release, but doing the compat on the H5Zblosc side is probably right

Copy link
Collaborator

@simonbyrne simonbyrne left a comment

Choose a reason for hiding this comment

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

I don't think it should be breaking, but I guess it could break version resolution?

There's already an HDF5 compat bound in H5Zblosc - that one just needs
to be kept up-to-date.
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.

3 participants