Use Path/PathBuf for fs paths

[Virgiel]

Fix #177 (comment)

[LouisGariepy]

I think we should use P: AsRef<Path> instead of &Path in our public APIs. For example, this makes it possible for users to supply &str instead of constructing a Path themselves.

I created a commit to this effect here.

Otherwise LGTM.

[Virgiel]

This pattern may become less ergonomic than it seems. Your proposed code change makes the code generic on a single type:

pub fn generate_live<P: AsRef<Path>>(
    client: &mut Client,
    queries_path: P,
    destination: Option<P>,
    settings: CodegenSettings,
)

This means that I can't use a String for queries_path and an Option<PathBuf for destination at the same time. To support this case, I need to be generic for each argument:

pub fn generate_live(
    client: &mut Client,
    queries_path: impl AsRef<Path>,
    destination: Option<impl AsRef<Path>>,
    settings: CodegenSettings,
)

Then, if I don't want to provide a destination, the Rust type system can no longer infer the generic type and I have to provide explicit typing for something I don't provide.

Since this code should only be called once in a build script, I'm not too picky about usability, and prefer to be explicit in this case.

If you still think that being generic on a single type is enough, I will accept your proposal.

[jacobsvante]

I agree that it's not the most important place for ergonomics. As long as the function accepts all valid OS paths then I give my thumbs up.

[LouisGariepy]

Since this code should only be called once in a build script, I'm not too picky about usability

Yeah you're right.

This means that I can't use a String for queries_path and an Option<PathBuf> for destination at the same time

If we use &Path everywhere, we also settle on one single type for all the fields, so there's no difference in terms of expressiveness. In any case, If one path is given as some type, its reasonable to assume that the user will provide all paths as this type, at least for the purpose of using our API.

My goal was to keep allowing users to provide paths as &str, which I think is a valid usecase and very common too. It also makes this change non-breaking (not a priority here, but still).

I'll leave it up to you, its not a big deal either way since as you mentioned this is only for a handful of paths specified once.

[LouisGariepy]

Otherwise looks good.

Not related to the goal of this PR, but I appreciate the small changes you've made to avoid passing around large structs by value, or cloning potentially big strings etc. Always appreciated :)

[LouisGariepy]

@Virgiel I'm curious why you prefer a separate path generic for schema paths?

<P: AsRef<Path>, F: AsRef<Path>>

I doubt this is necessary in practice, but perhaps I'm missing something? The PR is ready to be merged in any case, I'm just wondering.

[Virgiel]

@LouisGariepy
When we call generate_managed we have owned types String or PathBuff and owned collection Vec<Owned>. When we pass by reference, they become borrowed types &str or &Path and borrowed slices of owned types &[Owned]. So we want to be able to pass &str and &[String] at the same time and need two different generic types.

[LouisGariepy]

I think you're referring to our CLI, for example, where we have PathBuf and Vec<PathBuf>. But that's fine, we can still do

generate_managed(
        queries_path, // PathBuf
        &schema_files, // Slice of PathBuf
        Some(destination), // PathBuf
        <...>
)

So P = PathBuf and we don't need another generic.
Passing PathBuf by value is not inefficient since its already reference-sized under the hood.

[Virgiel]

I think you're referring to our CLI, for example, where we have PathBuf and Vec<PathBuf>. But that's fine, we can still do

generate_managed(
        queries_path, // PathBuf
        &schema_files, // Slice of PathBuf
        Some(destination), // PathBuf
        <...>
)

So P = PathBuf and we don't need another generic. Passing PathBuf by value is not inefficient since its already reference-sized under the hood.

You are right we can simply pass PathBuf by value 😅

[LouisGariepy]

💯 Ready to merge