JuliaIO / JLD2.jl

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

TimeDatatype not defined #465

Closed dpinol closed 1 year ago

dpinol commented 1 year ago

macro read_datatype in datatypes.jl uses TimeDatatype, which is not defined anywhere.

The issue is reported by JET

using JET
report_package(JLD2;mode=:typo)
...
═════ 20 possible errors found ═════
┌ @ /dani/dev/julia/JLD2.jl/src/JLD2.jl:365 JLD2.load_file_metadata!(f)
│┌ @ /dani/dev/julia/JLD2.jl/src/JLD2.jl:381 JLD2.close(f)
││┌ @ /dani/dev/julia/JLD2.jl/src/JLD2.jl:499 f.root_group["_types"] = f.types_group
│││┌ @ /dani/dev/julia/JLD2.jl/src/groups.jl:133 JLD2.prewrite(g.f)
││││┌ @ /dani/dev/julia/JLD2.jl/src/JLD2.jl:453 JLD2.load_datatypes(f)
│││││┌ @ /dani/dev/julia/JLD2.jl/src/JLD2.jl:439 JLD2.jltype(f, cdt)
││││││┌ @ /dani/dev/julia/JLD2.jl/src/data/reconstructing_datatypes.jl:24 JLD2.read_shared_datatype(f, cdt)
│││││││┌ @ /dani/dev/julia/JLD2.jl/src/datatypes.jl:444 JLD2.jlread(io, JLD2.CompoundDatatype)
││││││││┌ @ /dani/dev/julia/JLD2.jl/src/datatypes.jl:281 JLD2.jlread(io, JLD2.VariableLengthDatatype)
│││││││││┌ @ /dani/dev/julia/JLD2.jl/src/datatypes.jl:321 JLD2.jlread(io, JLD2.TimeDatatype)
││││││││││ `JLD2.TimeDatatype` is not defined
....
JonasIsensee commented 1 year ago

ha, you're right. I remember finding this myself at some point. HDF5 has a built-in time datatype that apparently no-one ever uses and I half-built support for it. Not really sure what's best here. One could properly implement it with it most likely never being used or one could remove the type with a proper "NotImplementedError"

dpinol commented 1 year ago

raising NotImplementedError sounds fine. I can create a MR throwing NotImplementedError from read_datatype in you find it ok

JonasIsensee commented 1 year ago

Please do! :) In fact, a UnsupportedFeatureException("some explanation") would probably be best.

dpinol commented 1 year ago

@JonasIsensee done!