GregoryConrad/rearch-rs

1.0.0 (for just ReArch)

GregoryConrad opened this issue · 0 comments

This issue only covers the 1.0.0 for the rearch crate. The other crates are not close to a 1.0, mostly due to limitations in the Rust compiler.

TODO:

  • Get a few code reviews of the library
  • Garner more feedback, ideally from several users adopting the library
  • Ideally some comments on my decisions in #31
    • Now irrelevant since CapsuleKey was rewritten as trait alias, and Capsule::key just returns an impl Hash + Eq + ...
  • Consider removing SideEffectRegister::register and instead have people call effect().build(register) for until experimental-api stabilizes
    • Yes, this does suck a little, but it also ensures there won't be any redundancy once it does stabilize
    • Update: I'm currently inclined to just keep it and deprecate it when no longer needed
  • Decide on the naming for CapsuleReader::get and CapsuleReader::as_ref
    • I'm not sure I like either that much as-is. get is redundant/non-descriptive, and as_ref breaks Rust naming conventions.
    • Update: I removed get.get. I am keeping get.as_ref as-is for now, but am looking for new naming suggestions.
    • Note that the final api ideally will be like get.as_ref(some_capsule) and get(some_capsule) (for returning a clone)
  • Should the Container/ContainerReadTxn/ContainerWriteTxn's xyz_read and xyz_read_ref be named as such?
    • Again, maybe we should encourage the low-cost options by default so we can delete the clone variants and then (maybe) just xyz_read_ref -> xyz_read?
    • Update: non-issue, since all methods were moved under the experimental-txn feature
  • Consider using coroutines for the API instead of CapsuleReader and the less-ergonomic SideEffectRegistrar
    • The issue is that this may be a 2.0.0 feature since who knows how far away coroutines are
    • I worry about how this will work with an LSP. Such a proc-macro really defies what the analyzer is expecting.
    • This would enable a syntax resembling (ideally without a macro, but idk if that is feasible):
#[capsule]
fn count_manager() -> (u32, impl CData + Fn()) {
  let (state, set_state) = yield effects::State::<Cloned<_>>(1234);
  (state, move || set_state(state + 1))
}

#[capsule]
fn count_capsule() -> u32 {
  yield Get(count_manager).0
} 
  • Maybe rename Capsule to TraditionalCapsule to accommodate the future addition of coroutines (to prevent breaking change or ugly API), and then have a private Capsule trait that TraditionalCapsule has a blanket impl for.
  • Make Capsule (or TraditionalCapsule) take From<CapsuleHandle> as input so that capsules can be defined with just CapsuleReader or SideEffectRegistrar parameters.
    • Not possible because the From<CapsuleHandle> results in an unconstrained type parameter.
  • Maybe make Container::with_read_txn, Container::with_write_txn, ContainerReadTxn, and ContainerWriteTxn crate private for a 1.0.0 to allow for extensibility or changes in the future
    • Although I do understand the use-cases for making them public for others to interop with.
    • Maybe we can make an "experimental" feature so people that want to use them probably can, at the cost of having to pin ReArch to a particular version.
    • Update: moved all of these APIs under the experimental-txn feature.
  • Test MockCapsuleReaderBuilder