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

Rename public functions and stop exporting several public API methods #699

Merged
merged 5 commits into from
Nov 12, 2020

Conversation

musm
Copy link
Member

@musm musm commented Oct 9, 2020

Rename public functions and stop exporting most public API

Open to re-exporting API people think should be exported.

@musm musm marked this pull request as draft October 9, 2020 05:41
@jmert
Copy link
Contributor

jmert commented Oct 9, 2020

Should the modifying functions terminate with ! to better follow typical Julia naming? IO functions don't, but Dict functions do, so not sure which should be followed.

@musm
Copy link
Member Author

musm commented Oct 10, 2020

A lot of these feel like we should just use multiple dispatch for, e.g.:

create_group(...) could all just be
create(HDF5.group, ....) et al. 

although the dispatch is messy because the function signatures vary quite a bit between them all.

@musm
Copy link
Member Author

musm commented Oct 10, 2020

Should the modifying functions terminate with ! to better follow typical Julia naming? IO functions don't, but Dict functions do, so not sure which should be followed.

That's a good point, I don't have a very strong opinion on this either, but it might make sense to adopt the Dict like notion for some of the functions. In particular delete, which AFAIK doesn't have a IO function equivalent.

@musm
Copy link
Member Author

musm commented Oct 10, 2020

although the dispatch is messy because the function signatures vary quite a bit between them all.

Eek, this in general might just be too confusing.

@musm
Copy link
Member Author

musm commented Oct 19, 2020

Note so I don't forget: figure out what to do with name, filename

@musm
Copy link
Member Author

musm commented Oct 26, 2020

I do think we should probably not export anything and make the public interface module scoped.

@musm musm mentioned this pull request Nov 1, 2020
6 tasks
@musm
Copy link
Member Author

musm commented Nov 3, 2020

If we are renaming these we should really decide what to unexport, otherwise it's going to be basically impossible to unexport the names if we want to do so in a deprecation friendly manner.

@musm
Copy link
Member Author

musm commented Nov 3, 2020

One option is to basically unexport everything other than h5* functions. Then if we decide to export module scoped public API functions, it will at least be possible to do so.

@jmert jmert mentioned this pull request Nov 3, 2020
@musm
Copy link
Member Author

musm commented Nov 3, 2020

Another question, which form should we adopt:
(1) group_write or (2) write_group

(2)reads more naturally and is the scheme adopted by, e.g. h5py, and more consistent in general with Julia. OTOH, (1) allows you to hit <Tab> on group_ and easily discover all available methods.

@musm musm force-pushed the renam branch 3 times, most recently from bb1785f to dd196c2 Compare November 4, 2020 23:57
musm added a commit to musm/HDF5.jl that referenced this pull request Nov 4, 2020
musm added a commit to musm/HDF5.jl that referenced this pull request Nov 5, 2020
musm added a commit to musm/HDF5.jl that referenced this pull request Nov 5, 2020
src/show.jl Outdated Show resolved Hide resolved
@musm musm marked this pull request as ready for review November 5, 2020 20:29
@musm musm changed the title Rename public functions Rename public functions and stop exporting most public API Nov 5, 2020

import Libdl
import Mmap

# needed for filter(f, tuple) in julia 1.3
using Compat

export
@read, @write,
Copy link
Member Author

Choose a reason for hiding this comment

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

We should change this to @load and @save to align with JLD and JLD2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably follow up work.

src/HDF5.jl Outdated Show resolved Hide resolved
@musm
Copy link
Member Author

musm commented Nov 6, 2020

This should be ready for review now.

What do we think about #699 (comment) ?

In a follow up I propose we remove/refactor h5rewrite, h5writeattr, h5readattr

@musm musm requested a review from kleinhenz November 6, 2020 21:21
@musm musm requested review from jmert and removed request for kleinhenz November 6, 2020 21:21
src/deprecated.jl Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
src/HDF5.jl Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
@jmert
Copy link
Contributor

jmert commented Nov 10, 2020

In a follow up I propose we remove/refactor h5rewrite, h5writeattr, h5readattr

👍

@musm musm changed the title Rename public functions and stop exporting most public API Rename public functions and stop exporting several public API methods Nov 12, 2020
@musm
Copy link
Member Author

musm commented Nov 12, 2020

Thanks for the review. Addressed the comments and fixed up the remaining issues.

Do you want to sign off on the updated export list?

(Should we export ishdf5?)

@jmert
Copy link
Contributor

jmert commented Nov 12, 2020

(Should we export ishdf5?)

I'm agnostic — I think there's no harm in exporting it, but I haven't actually personally found it necessary either. (I've been using FileIO.query on files and dispatching on FileIO.File{format"HDF5"}.)

@jmert
Copy link
Contributor

jmert commented Nov 12, 2020

Just as a note, I found I temporarily needed:

diff --git a/src/deprecated.jl b/src/deprecated.jl
index 04e204a..fa138f7 100644
--- a/src/deprecated.jl
+++ b/src/deprecated.jl
@@ -412,18 +412,18 @@ end
 @deprecate objinfo(obj::Union{File,Object}) object_info(obj)
 
 # - rename bindings
-@deprecate a_open    open_attribute
-@deprecate a_read    read_attribute
-@deprecate a_write   write_attribute
-@deprecate a_create  create_attribute
-@deprecate a_delete  delete_attribute
-@deprecate attrs     attributes
+@deprecate a_open                    open_attribute
+@deprecate a_read                    read_attribute
+@deprecate a_write(args...; kws...)  write_attribute(args...; kws...)
+@deprecate a_create(args...; kws...) create_attribute(args...; kws...)
+@deprecate a_delete                  delete_attribute
+@deprecate attrs                     attributes
 
-@deprecate d_open            open_dataset
-@deprecate d_read            read_dataset
-@deprecate d_write           write_dataset
-@deprecate d_create          create_dataset
-@deprecate d_create_external HDF5.create_external_dataset
+@deprecate d_open                    open_dataset
+@deprecate d_read                    read_dataset
+@deprecate d_write(args...; kws...)  write_dataset(args...; kws...)
+@deprecate d_create(args...; kws...) create_dataset(args...; kws...)
+@deprecate d_create_external         HDF5.create_external_dataset
 
 @deprecate g_open   open_group
 @deprecate g_create create_group
@@ -432,7 +432,7 @@ end
 @deprecate o_copy   copy_object
 @deprecate o_delete delete_object
 
-@deprecate p_create create_property
+@deprecate p_create(args...; kws...) create_property(args...; kws...)
 
 @deprecate t_create create_datatype
 @deprecate t_open   open_datatype

to work through updating my MAT.jl's HDF5_v0.14-branch for this PR because it was already updated to property syntax and the bare @deprecate doesn't add keywords automatically. I don't necessarily think this should be added, though, because it's not relevant for upgrading from the tagged v0.13.6 — rather just wanted to point it out as an interesting consequence of multiple rounds of deprecated changes.

@musm
Copy link
Member Author

musm commented Nov 12, 2020

Ok great, thanks again!

@musm musm merged commit aafbeaf into JuliaIO:master Nov 12, 2020
@musm musm deleted the renam branch November 12, 2020 18:13
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