libpathrs icon indicating copy to clipboard operation
libpathrs copied to clipboard

make libpathrs Rust configuration sane

Open cyphar opened this issue 5 years ago • 1 comments

The current method of configuration is a really bad idea (exposing structure internals of Root is going to cause problems, I can feel it in my bones). So we should instead use a different design, but it's unclear to me what the best approach is:

  • Just have methods to set (and possibly get) configuration of the object. It's not the nicest thing in the world, but it's fairly workable.

  • The most common approach is the "builder" method. The issue with it is whether there should be a way to modify the configuration after you've consumed the builder. If yes, then what is the point of the builder (you could just modify the configuration and not have a builder -- which is the alternative proposal). If no, and then how do we expose the builder method through the C API -- we'd need to have C builders which is just going to be absolutely awful.

Some follow-on questions (and my tentative answers):

  • Do people really care about modifying the configuration after the fact?

    • Maybe, I'm not too sure.
  • How do we deal with the try_clone case and modifying the configuration of the copy (ditto for from_file_unchecked)?

    • We could do it by making the builder take a variety of "source" arguments (so you can source from a directory -- creating a new builder -- or from an existing object or file handle). The configuration could be pre-loaded from the existing object if applicable. Downside is that the API would be something like:
let root = RootBuilder::new().source("/some/path").build()?;
let root = RootBuilder::new().source_file_unchecked(root_file).build()?;
// how do we do try_clone?
let root = old_root.try_clone()?.build()?; // ?!
let root = RootBuilder::new().source_root(old_root).build()?; // !?
let root = RootBuilder::new().source_file_unchecked(old_root.try_clone()?.into_file()).build()?; // lolno

cyphar avatar Jan 25 '20 16:01 cyphar

Or maybe you could make the API for try_clone be the same as it currently is, but there is a way you can consume a Root (or Handle) and turn it into a builder?

let root = old_root.try_clone()?.rebuild().set_some_config().build()?; // maybe?

The other problem is what should Root::resolve return? A builder? Does that really make too much sense? Handles don't really have a nice point where you can insert a constructor-like setup...

cyphar avatar Jan 25 '20 16:01 cyphar