Closed Virgiel closed 1 year ago
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.
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.
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.
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.
@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.
@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.
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.
I think you're referring to our CLI, for example, where we have
PathBuf
andVec<PathBuf>
. But that's fine, we can still dogenerate_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 😅
:100: Ready to merge
Fix https://github.com/cornucopia-rs/cornucopia/pull/177#issuecomment-1313208321