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

File constructor / FileBuilder API rework #49

Merged
merged 15 commits into from
Jul 18, 2019
Merged

File constructor / FileBuilder API rework #49

merged 15 commits into from
Jul 18, 2019

Conversation

aldanor
Copy link
Owner

@aldanor aldanor commented Jul 17, 2019

This is certainly a breaking change, but better sooner than later (initial discussion with @Enet4 a while ago in #14).

Main points:

  • FAPL and FCPL bindings are pretty much 100% complete, it's a shame they couldn't have been used until now; this is now fixed, the new FileBuilder has full access to those (!).
  • String-based modes like "r" and "w" are gone. Instead, you can just use File::open(), File::open_rw(), File::create(), File::create_excl() and File::append() (FileBuilder has those methods two as finishers).
  • In case of need, there's also File::open_as() which mimics previous behaviour, but is strongly typed and doesn't use string-based modes either (FileBuilder has it too).
  • Special cases like driver strings etc are gone from the new FileBuilder, instead you can access FAPL and FCPL builders from it in a few different ways (by importing an existing plist, by getting a mutable reference, or by providing a closure that does something).

Example 1:

// before
File::open("foo.h5", "r+")

// after
File::open_rw("foo.h5")

Example 2:

// before
File::with_options().mode("w").driver("core").filebacked(false).open(&filename)

// after
File::with_options().with_fapl(|p| p.core_filebacked(false)).create(&filename)

@SuperFluffy
Copy link
Contributor

I wonder why you are not opting to represent the file constructor similar std::fs::OpenOptions?

@aldanor
Copy link
Owner Author

aldanor commented Jul 18, 2019

@SuperFluffy

Because it's simpler this way, I think? Not everything from there makes sense here (e.g. read is always enabled; append doesn't really have the same meaning etc; exclusive write is a new thing here) and as such blindly duplicating it doesn't make sense either.

Basically, my thought was - in fs::OpenOptions there's 6 options and 1 finalizer, open(). Multiple combinations of those options are valid for normal file system, so there's up to 64 variants - some of which make sense, some don't, but what's important is that the number of valid options is certainly greater than 6.

Here, there's a few modes pre-defined in HDF5 and that's it ("append" synthetic mode is actually stolen from h5py and is just a convenient way of doing open-otherwise-create, it's not a built-in). Combining them doesn't make any sense whatsoever. So... instead of having 4/5 options and one finalizer, why not just have 5 finalizers (plus 1, the open_as()).

Plus, File gains the same finalizers (as constructors), allowing you to do simply

File::open_rw("foo.h5")

without delving into docs of FileBuilder in order to figure out the chain of mutations you have to apply first. So, the user would only have to touch the builder if they want to change FAPL or FCPL - which is a much rarer case than just adjusting the file access mode.

@aldanor
Copy link
Owner Author

aldanor commented Jul 18, 2019

Anyhow, while this may not be the final iteration (if anyone has strong opinions on how the API should look like), I'm strongly convinced that the proposed version is better than using "r" string to indicate reading and "core" string to indicated the core driver - which is what we have in the current master.

Here everything's strongly typed, and the most frequent use cases (open a file, create a new one, etc) become subjectively more simple and elegant.

@aldanor aldanor merged commit c05dd03 into master Jul 18, 2019
@aldanor aldanor deleted the feature/file-api branch August 9, 2020 18:38
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.

None yet

2 participants