connorskees / grass

A Sass compiler written purely in Rust
https://docs.rs/grass/
MIT License
499 stars 38 forks source link

Unnecessary use of `Path` objects in `Options` API (and makes the API significantly harder to use) #64

Closed colbyn closed 1 year ago

colbyn commented 2 years ago

This api is kinda frustrating and the lifetimes are unnecessary, it should something akin to

pub fn load_paths<P: AsRef<Path>>(mut self, paths: &[P]) -> Self {…}
pub fn load_path<P: AsRef<Path>>(mut self, path: P) -> Self {…}

This is more akin to how APIs typically work in the rust std. The overhead is probably insignificant, and makes the Options API significantly easier to use, and lets your end-users pass in anything thats convertible to e.g. a Path (via reference) or a PathBuf (as an owned value).


Personally, regarding the Options struct over here, it'd be simpler to just store owned values (and so use PathBuf), e.g. instead of:

#[derive(Debug)]
pub struct Options<'a> {
    load_paths: Vec<&'a Path>,
    …
}

It should be

#[derive(Debug)]
pub struct Options<'a> {
    load_paths: Vec<PathBuf>,
    …
}

And just copy the values pass in VIA

pub fn load_paths<P: AsRef<Path>>(mut self, paths: &[P]) -> Self {…}
pub fn load_path<P: AsRef<Path>>(mut self, path: P) -> Self {…}

But, if you really wanna use Path objects and avoid copying, I'd use Cow<Path> pointers internally and then give the user the option of passing in owned values or references VIA e.g. P: AsRef<Path> or P: Into<PathBuf> (for passing in owned values specifically or just P: AsRef<Path> and clone it).

connorskees commented 1 year ago

Thank you for this suggestion. This has been implemented in 31bbe8f and will be included in the next minor release at some point this week.