implemented the builder api for salsa inputs #418
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
| let input_index = self.input_index(); | ||
| let field_indices = self.all_field_indices(); | ||
|
|
||
| let buidler: syn::ItemStruct = parse_quote! { |
There was a problem hiding this comment.
Typo
| let buidler: syn::ItemStruct = parse_quote! { | |
| let builder: syn::ItemStruct = parse_quote! { |
| } | ||
| }; | ||
|
|
||
| (buidler, impls) |
There was a problem hiding this comment.
| (buidler, impls) | |
| (builder, impls) |
| let build_fn: syn::ImplItemMethod = if self.0.is_isingleton() { | ||
| parse_quote! { | ||
| fn build(&mut self, __db: &#db_dyn_ty) -> std::result::Result<#ident, std::boxed::Box<dyn std::error::Error>> { | ||
| let durability = self.durability.ok_or_else(|| "durability not provided")?; |
There was a problem hiding this comment.
Why panic when a durability isn't specified, instead of using the default of low? I think one of the main advantages of a builder API is that you can have a lot of options, most of which aren't required, and you can only make the changes you need over sensible defaults.
There was a problem hiding this comment.
Hmmn true. Thanks. Will fix
| let __id = __ingredients.#input_index.new_singleton_input(__runtime); | ||
|
|
||
| #( | ||
| __ingredients.#field_indices.store_new(__runtime, __id, self.#field_names.clone().ok_or_else(||"field not provided")?, durability); |
There was a problem hiding this comment.
I don't think using the same durability for all the fields gives enough flexibility to the user. Wouldn't it be better to have a method with_durabilities which works like with_fields? The current with_durability method could stay, but I'm not sure if the name isn't slightly ambiguous, because we have multiple fields all with their own durability.
There was a problem hiding this comment.
Thought about having with_durabilities too, but I dropped the idea because it sorts of requires the user to keep the order of fields in mind. I don't know if that cognitive overhead is okay. @MihailMihov, @nikomatsakis , @xffxff
Though personally, I think it's better.
Another option I'm considering is having with_field, and not with_fields, so we get to set the fields one by one and take a tuple, i.e. its value and it's durability as arguments. That localizes concerns. But, is taking tuples neat? Also, is setting the fields one by one okay?
Thanks Mihail!
There was a problem hiding this comment.
Thought about having with_durabilities too, but I dropped the idea because it sorts of requires the user to keep the order of fields in mind. I don't know if that cognitive overhead is okay.
The cognitive overhead point is something that I didn't consider, but I did go over the idea with the tuples and decided to not suggest it, because it wouldn't look very good if we needed to add more options for the fields in the future. And then we also lose the ability to change just 1 or 2 things about a field, we'd need to set all 5 (again this only applies if we do consider having more than just a value and a durability).
Also if tuples were an option all the intermediate structs produced by the builder would have an incomplete state, and the user would need to keep track of all the fields being set.
But that's just my opinion I think we should hear what others have to say.
| self | ||
| } | ||
|
|
||
| fn with_fields(&mut self, #(#field_names: #field_tys,)*) -> &mut Self { |
There was a problem hiding this comment.
with_values would be a better name in my opinion, but the current is fine too.
There was a problem hiding this comment.
Hmmn. with_fields makes it obvious to the user what they're about to set. Wouldn't with_values be a little ambiguous? I really don't know.
There was a problem hiding this comment.
I think of a field as something which has a value, a durability and maybe other things associated with it, which is why with_values is better in my opinion as it communicates that you're setting just the value of the field. If I saw with_fields my first thought would be that it takes a struct or tuple which contains everything about a field. Maybe someone else could comment on what is the better option, but I think both are good?
| .with_durability(Durability::HIGH) | ||
| .with_fields(20, 40) | ||
| .build(&db) | ||
| .unwrap(); |
There was a problem hiding this comment.
Returning a Result from build isn't useless, but I also feel like almost always having to unwrap it will be something very repetitive and rarely useful. If you agree with my suggestion that the durability should have a default I have a different idea, which I think will make the API better, instead of having the build method be the terminal one, we could instead move that over to with_fields and we'd no longer need to return a Result, because we know that we have the values and the other thing we need is the db, but there a panic would be fine.
let input_from_builder = MyInput::new_builder()
.with_db(&db)
.with_durability(Durability::HIGH)
.with_fields(20, 40);I think this makes the API calls shorter, with no downsides and it would be closer to the salsa::Setter API. The only thing that I kind of don't like is how to fields method is named similarly to the other ones, which doesn't clearly show that it's the terminal method that actually builds the type.
There was a problem hiding this comment.
The only thing that I kind of don't like is how to fields method is named similarly to the other ones, which doesn't clearly show that it's the terminal method that actually builds the type.
That's a real concern. I actually think builders should have build, and should naturally return Results except when the value we're building can comfortably have default values all through. In this case, fields cannot have default values. And, panicking inside with_fields? hmmn maybe people don't expect that. But I wouldn't know. I don't have much experience here. If you're so sure...
Maybe others have something to say?
@xffxff @nikomatsakis @crlf0710
There was a problem hiding this comment.
In this case, fields cannot have default values.
That and the database are the two things that we need to set for the struct to be complete. I think either one of those could be used as the build method and panic only if the other is missing, but it's a tradeoff where we make the API calls slightly shorter, but lose a bit of clarity of which method is the one that builds the type.
But panicking instead of returning Result and making either with_db or with_fields the build method are two separate things. If you're sure that the build should return Result you can still consider putting that logic in with_db or with_fields.
But I wouldn't know. I don't have much experience here. If you're so sure...
I don't either, so take what I say with a grain of salt. I just wanted to bring it up as a talking point, so we could discuss it with the others.
There was a problem hiding this comment.
Coming back to this with fresh eyes, a few thoughts--
- I think that we should strive to avoid
Resultthat are statically known to not apply. - I don't love having
with_valuestake a tuple, I think I'd rather have a method per field. - This seems to imply we may need to do "type tricks" to make a builder that is statically known to have a value for all fields etc.
It seems like we should back up from the PR and first we be more specific about the API that we want for an example.
|
@OLUWAMUYIWA I know I disappeared a long time here! I'm still interested in this PR, are you still interested to pursue it? (Totally understand if not) |
|
Hi @nikomatsakis I want to finish it up! I'll get on it. |
✅ Deploy Preview for salsa-rs canceled.
|
|
@OLUWAMUYIWA oh, awesome! |
|
What I've found really useful in the past is to write manually the code I expect to generate. Maybe you can make a gist with an example input and output and we can discuss that? Else I can do it for you if you prefer :) |
|
@OLUWAMUYIWA I'm still interested here, but note that we did some big renamings and things, so you'll definitely have to rebase etc. Given its age, I'm going to go ahead and close this PR-- please reach out on Zulip or comment in the GIthub issue though if you want to discuss next steps. |
Ref: #385