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

Add deprecated bindings for backwards compatibility with ecosystem packages #655

Merged
merged 1 commit into from
Sep 2, 2020

Conversation

jmert
Copy link
Contributor

@jmert jmert commented Sep 1, 2020

Namely, get the tests of JLD.jl and MAT.jl to pass again.

As noted in the new deprecated.jl file, PRs #629 and #632 have broken JLD.jl and MAT.jl for current HDF5.jl master. This PR restores them to working order by adding a bunch of deprecation methods.

Of particular note is re-adding default property values to the d_create method — JLD.jl was already using the method and relying on the fact that it only had to provide 2 out of the 4 property lists. I'm also preparing a PR to MAT.jl which ends up using this method as well to fix an error it has after #631.

PRs #629 and #631 have already been tagged in minor releases v0.13.3 and v0.13.4, respectively, so it'd be nice to get some form of deprecated backwards-compatibility layer in place so that until a major version bump is made, external packages continue to work.

@musm
Copy link
Member

musm commented Sep 2, 2020

@jmert thanks for this !

I created a release-0.13 branch, where I dropped several of the newer commits on master such as #632 and related commits that require a major version bump.

In particular, if we could get the deprecations for PRs #629 and #631 into release-0.13 branch, I believe that should be sufficient for a new patch tag.

@jmert
Copy link
Contributor Author

jmert commented Sep 2, 2020

That sounds good. Would you like me to split this PR up into two — this one with just the changes relevant to the v0.13 branch, and then take the deprecations for #632 and make that a separate PR against master?

(Related to that, I could backport my #645 change to the v0.13 branch since I'd like to use that functionality sooner rather than later :-) )

@musm
Copy link
Member

musm commented Sep 2, 2020

(Related to that, I could backport my #645 change to the v0.13 branch since I'd like to use that functionality sooner rather than later :-) )

👍

That sounds good. Would you like me to split this PR up into two — this one with just the changes relevant to the v0.13 branch, and then take the deprecations for #632 and make that a separate PR against master?

👍

…ckages

Namely, get the tests of JLD.jl and MAT.jl to pass again.
@jmert jmert changed the base branch from master to release-0.13 September 2, 2020 16:42
@jmert
Copy link
Contributor Author

jmert commented Sep 2, 2020

OK, I think I've rebased this one onto the release-0.13 branch correctly — both MAT.jl and JLD.jl's tests passed for me locally.

@musm
Copy link
Member

musm commented Sep 2, 2020

great. Would be good if those packages got updated in the future.

@musm musm merged commit a629a3e into JuliaIO:release-0.13 Sep 2, 2020
@jmert
Copy link
Contributor Author

jmert commented Sep 2, 2020

That sounds good. Would you like me to split this PR up into two — this one with just the changes relevant to the v0.13 branch, and then take the deprecations for #632 and make that a separate PR against master?

+1
...
OK, I think I've rebased this one onto the release-0.13 branch correctly — both MAT.jl and JLD.jl's tests passed for me locally.

I just realized that I did this backwards, but oh well — this PR should have stayed against master since it will need all deprecations (to give packages a clear upgrade path to v0.14), and then a new PR should have contained only the deprecations due to #629 to fix the v0.13 compatibility.

New PR incoming for master.

@jmert jmert deleted the depwarn_compat branch September 2, 2020 16:56
@musm
Copy link
Member

musm commented Sep 2, 2020

Should I wait before creating a new patch release on 'release-0.13'?

@jmert
Copy link
Contributor Author

jmert commented Sep 2, 2020

No, I think release-0.13 is OK to go. I just had to do a bit of fiddling around to get a sane master PR again.

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.

2 participants