Library API and idiomatic-ness review

[mrr]

Hi All!

I recently updated and released version 0.2 of the planif crate (github) which I believe is now feature complete.

I would appreciate any comments about the library's api since it is the first library crate I've released.

Also, any tips in general to make the code more idiomatic would be great.

I did manage to make clippy panic when I ran it on the code, so that's something.

Thanks in advance!

[Michael-F-Bryan]

@mrr

I would appreciate any comments about the library's api since it is the first library crate I've released.

The biggest recommendation I would make at the moment is to add #![deny(missing_docs)] to the top of your lib.rs file.

At the moment, it's impossible to say whether your library would satisfy my needs without looking through the source code because none of the modules and most of the types don't have any documentation.

Docs are arguably more important to an end user than feature completeness... if something isn't discoverable, it may as well not exist

---

mattrobineau/planif/blob/ac01bae6dec544f21006b50ff5840523bf381336/src/schedule_builder.rs#L348-L355

This uninitialize() method smells funny to me.

If you've got some resource that needs to be explicitly initialized and cleaned up when you are done with it (i.e. COM) then it should be put behind some sort of RAII guard that is passed to anything needing access to it.

For example, you might create something like this:

#[derive(Clone)]
pub struct ComRuntime(Rc<ComRuntimeInner>);

struct ComRuntimeInner;

impl ComRuntimeInner {
  fn initialize() -> Result<Self, Box<dyn Error>> {
    unsafe { CoInitializeEx(std::ptr::null_mut(), COINIT_MULTITHREADED)?; }
    Ok(ComRuntimeInner)
  }
}

impl Drop for ComRuntimeInner {
  fn drop(&mut self) {
    unsafe { CoUninitialize(); }
  }
}

Then any functions needing access to COM accepts it as an argument and types that depend on the COM runtime being initialized should store it as a field so they can't accidentally be used after CoUninitialize() is called.

struct ScheduleBuilder<T> {
  com: ComRuntime,
}

impl<T> ScheduleBuilder<T> {
  pub fn new(com: &ComRuntime) -> Result<Self, Box<dyn Error>> {
    let com = com.clone();
    ...
    Ok(ScheduleBuilder { com, ... })
  }
}

You can also use references instead of an Rc, but lifetimes are infectious and having your API littered with 'co lifetimes will be unergonomic. You also tend to run into self-referential struct situations because end users will often want to store the ComRuntime in some top-level struct alongside the things that are using it.

---

Other than copy/paste, is there a better way you could represent the way some frequencies allow you to delay or remotely trigger them, while others don't?

There are quite a few methods which are only available for certain types, so maybe you could use some sort of trait and a where-clause.

impl<F> ScheduleBuilder<F> 
  where F: Delayable
{
  fn delay(&self, duration: &str) -> Result<Self, Box<dyn Error>> { ... }
}

trait Delayable { 
  fn delay(trigger: ITrigger, duration: &str) -> Result<(), Box<dyn Error>>; 
}

impl Delayable for Daily { ... }

---

mattrobineau/planif/blob/ac01bae6dec544f21006b50ff5840523bf381336/src/schedule_builder.rs#L738-L752

In Rust, we prefer to avoid stringly typed APIs where possible because it's super easy for users to mess up the PDTHMS format.

It might be a better idea to create some sort of Delay struct which can be formatted to create a PDTHMS string.

struct Delay {
  days: u64,
  hours: u16,
  minutes: u16,
  seconds: u16,
}

// provide a convenient way to construct a delay
impl Delay {
  fn days(days: u64) -> Self { Delay { days, hours: 0, minutes: 0, seconds:0 } }
  fn with_days(self, days: u64) -> Self { Delay { days, ..self } }
  ...
}

impl From<Duration> for Delay { ... }

impl Display for Delay {
  fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
    // write a PDTHMS string to the formatter
  }
}

impl FromStr for Delay {
  type Err = ...;
  fn from_str(s: &str) -> Result<Self, Self::Err> {
    // parse a PDTHMS string into a Delay (for completeness/convenience)
  }
}

---

There's also a fair amount of unsafe code littered throughout ScheduleBuilder. I know unsafe is unavoidable, but you might want to move it into the frequency types themselves. For example, you could make Daily and friends manage the ITrigger, and ScheduleBuilder just calls methods on it (e.g. using that Delayable trait from earlier).

That's the general idea behind contain unsafety in small modules in the Rust patterns book.

If you have unsafe code, create the smallest possible module that can uphold the needed invariants to build a minimal safe interface upon the unsafety. Embed this into a larger module that contains only safe code and presents an ergonomic interface.

[mrr]

Thank you @Michael-F-Bryan for the insightful and very detailed feedback. This is exactly what I was looking for.