JuliaIO / MAT.jl

Julia module for reading MATLAB files
MIT License
280 stars 71 forks source link

Deprecate exists to haskey and names to keys and bump minor version in prep for HDF5 release #156

Closed musm closed 3 years ago

jmert commented 3 years ago

I think we should also do names -> keys at the same time — I've pushed a commit to do that.

musm commented 3 years ago

I checkout out this MAT branch and the JLD branch with the depwarn removal.

(@v1.7) pkg> activate --shared dev
  Activating environment at `C:\Users\Mus\.julia\environments\dev\Project.toml`

julia> using MAT,JLD
[ Info: Precompiling JLD [4138dd39-2aa7-5051-a626-17a0bb65d9c8]

julia> mfile = matopen("../MAT/test\\v7\\struct.mat")
MAT.MAT_v5.Matlabv5File(IOStream(<file ../MAT/test\v7\struct.mat>), false, #undef)

julia> exists(mfile,"s")
WARNING: JLD.exists is deprecated, use haskey instead.
  likely near REPL[5]:1

I think the only thing we need to ensure is that we tag JLD and MAT at the same time with the removal, unless I'm missing something.

musm commented 3 years ago

Oh I see, they would both export their own exists binding, so the error message would be wrong, but the method call should be correct as far as I can tell. So is that the issue?

And this would only be a problem for users importing both the MAT, JLD packages at the same time.

jmert commented 3 years ago

I think that accidentally works — note that it says JLD.exists is deprecated, not MAT.exists. Since the @deprecate_binding adds untyped methods, though, any exported binding is sufficient to forward to the common Base.haskey, and then type dispatch works as expected.

If both MAT and JLD are upgraded simultaneously, then there's no functional problem (though still a confusing error message), but if you get a mismatch of MAT v0.10 (next release) and JLD v0.11 (current latest), then the deprecations break, I think.

jmert commented 3 years ago

And this would only be a problem for users importing both the MAT, JLD packages at the same time.

Yeah, we're both replying about the same time but reaching the same conclusion — I think the key is to maintain compatibility across any valid combination of package versions, which could include with and without deprecations in MAT and JLD (or vice versa), respectively.

(Though both package restricting themselves to HDF5 v0.15 on a release would serve to synchronize package compatibility "eras". The need for a deprecation period makes that probably undesirable for the next release.)

musm commented 3 years ago

Ok so if I understand things correctly.

And correct me if this is wrong, but our options are:

option 1:

  1. Modify this and the JLD PR to bump the HDF5 version to "v0.15" in Project.toml
  2. Bump HDF5 to "v0.15"
  3. Immediately, merge the MAT and JLD PRs to prevent an upgrade whereby older MAT and JLD version pull in HDF5 v0.15, where HDF5.exist has been excised.

option 2:

  1. Merge this PR as is.
  2. Don't remove HDF5.exists in the "v0.15" release.
  3. In the next MAT,JLD minor version bump remove the deprecations and restrict HDF5 to v0.15.
  4. Remove HDF5.exists in the v0.16 cycle of HDF5.

If this is correct, my preference is for option 1, as it seems to require fewer steps where one could screw up in the process or forget by the team someone gets to all the stages along the timeline. Open to either option, regardless.

jmert commented 3 years ago

Ok so if I understand things correctly.

And correct me if this is wrong, but our options are:

option 1:

  1. Modify this and the JLD PR to bump the HDF5 version to "v0.15" in Project.toml
  2. Bump HDF5 to "v0.15"
  3. Immediately, merge the MAT and JLD PRs to prevent an upgrade whereby older MAT and JLD version pull in HDF5 v0.15, where HDF5.exist has been excised.

The compat bounds in Project.toml should prevent this from happening, shouldn't it?

option 2:

  1. Merge this PR as is.
  2. Don't remove HDF5.exists in the "v0.15" release.
  3. In the next MAT,JLD minor version bump remove the deprecations and restrict HDF5 to v0.15.
  4. Remove HDF5.exists in the v0.16 cycle of HDF5.

If this is correct, my preference is for option 1, as it seems to require fewer steps where one could screw up in the process or forget by the team someone gets to all the stages along the timeline. Open to either option, regardless.

Option 3:

  1. Merge this PR (and similar for JLD).
  2. Release new major versions of MAT and JLD with deprecations (still using HDF5 v0.14).
  3. Release HDF5 v0.15 with HDF5.exists (and other deprecations) removed.
  4. Remove the deprecations from MAT and JLD, and bump requirement to HDF5 v0.15
  5. Release another major version bump for MAT and JLD, each.
musm commented 3 years ago

The compat bounds in Project.toml should prevent this from happening, shouldn't it?

Oh your right. It's the caret specifier and for packages without a major digit it's the same as tilde, so we should be good.

musm commented 3 years ago

Do you have a preference for which option? I think Option 3 or Option 1 in order of preference.