Rename public exports to exclude HDF5 prefix #690
Conversation
|
Review comments addressed. |
|
What is the point of the |
It looks like really old legacy naming from very early Julia ?? — JuliaLang/julia@3ca22bd |
Good digging, probably that explains this 😄 |
|
I don't think this PR is controversial. Looking at the update, the code is so much cleaner and better IMO. |
|
I'll merge sans objections? |
| @@ -1381,8 +1380,8 @@ end | |||
| # Create datasets and attributes with "native" types, but don't write the data. | |||
| # The return syntax is: dset, dtype = d_create(parent, name, data; properties...) | |||
| for (privatesym, fsym, ptype) in | |||
| ((:_d_create, :d_create, Union{HDF5File, HDF5Group}), | |||
| (:_a_create, :a_create, Union{HDF5File, HDF5Group, HDF5Dataset, HDF5Datatype})) | |||
| ((:_d_create, :d_create, Union{File,Group}), | |||
There was a problem hiding this comment.
We might want to unroll these to improve precompile speeds?
There was a problem hiding this comment.
I agree, but I think it'd be OK to wait on for another PR.
There was a problem hiding this comment.
Definitely new PR but thought I'd mention it.
| @@ -85,13 +84,13 @@ hdf5_type_id(::Type{Int64}) = H5T_NATIVE_INT64 | |||
| hdf5_type_id(::Type{UInt64}) = H5T_NATIVE_UINT64 | |||
| hdf5_type_id(::Type{Float32}) = H5T_NATIVE_FLOAT | |||
| hdf5_type_id(::Type{Float64}) = H5T_NATIVE_DOUBLE | |||
| hdf5_type_id(::Type{HDF5ReferenceObj}) = H5T_STD_REF_OBJ | |||
| hdf5_type_id(::Type{Reference}) = H5T_STD_REF_OBJ | |||
|
|
|||
| hdf5_type_id(::Type{S}) where {S<:AbstractString} = H5T_C_S1 | |||
| hdf5_type_id(::Type{C}) where {C<:CharType} = H5T_C_S1 | |||
|
|
|||
| const HDF5BitsKind = Union{Bool, Int8, UInt8, Int16, UInt16, Int32, UInt32, Int64, UInt64, Float32, Float64} | |||
There was a problem hiding this comment.
These are used internally, but could also remove the HDF5 prefix.
There was a problem hiding this comment.
There's a couple of others like this including HDF5Scalar that are internally used.
There was a problem hiding this comment.
@jmert Should I save these changes in a new PR ?
There was a problem hiding this comment.
I'll do them in a separate PR since they are more internal utility constants, rather than user-facing ones, so logically it would make sense to be a separate PR.
| @@ -51,8 +51,8 @@ dims, max_dims = HDF5.get_dims(b) | |||
| @test dims == (UInt64(10000),) | |||
| @test max_dims == (HDF5.H5S_UNLIMITED,) | |||
| #println("b is size current $(map(int,HDF5.get_dims(b)[1])) max $(map(int,HDF5.get_dims(b)[2]))") | |||
| # b[:] = [1:10000] # gave error no method lastindex(HDF5Dataset{PlainHDF5File},), | |||
| # so I defined lastindex(dset::HDF5Dataset) = length(dset), and exported lastindex | |||
| # b[:] = [1:10000] # gave error no method lastindex(HDF5.Dataset{PlainHDF5File},), | |||
There was a problem hiding this comment.
Unrelated, but what's up with the type parameter here? I'm guessing just something long out of date?
There was a problem hiding this comment.
No idea, I noticed this too PlainHDF5File doesn't exist, probably old legacy code.
| @@ -1381,8 +1380,8 @@ end | |||
| # Create datasets and attributes with "native" types, but don't write the data. | |||
| # The return syntax is: dset, dtype = d_create(parent, name, data; properties...) | |||
| for (privatesym, fsym, ptype) in | |||
| ((:_d_create, :d_create, Union{HDF5File, HDF5Group}), | |||
| (:_a_create, :a_create, Union{HDF5File, HDF5Group, HDF5Dataset, HDF5Datatype})) | |||
| ((:_d_create, :d_create, Union{File,Group}), | |||
There was a problem hiding this comment.
I agree, but I think it'd be OK to wait on for another PR.
No description provided.