Capture doc comments, add variant and field builders #87
Conversation
ascjones
changed the title
Co-authored-by: Robin Freyler <[email protected]>
There was a problem hiding this comment.
I think the FieldsBuilder<N, T> state machinery is really clever, good job there.
The resulting API is a bit awkward to read but I'm not sure how difficult it is to do better. Also had a question about what it would take to always populate the discriminant member for variants.
| /// Add an unnamed field with the type of the type parameter `T` | ||
| pub fn field_of<T>(mut self, type_name: &'static str) -> Self | ||
| /// Type states for building a field. | ||
| pub mod field_state { |
There was a problem hiding this comment.
Can you elaborate on this choice? It couldn't be a FieldState enum I think, but the thought process behind this would be enlightening, what options did you consider?
There was a problem hiding this comment.
Using the type state builder pattern to ensure that only all named or all unnamed fields can be constructed. The other option is a runtime check, which would make for less code but better to catch things at compile time.
| pub struct VariantBuilder { | ||
| name: &'static str, | ||
| fields: Vec<Field<MetaForm>>, | ||
| discriminant: Option<u64>, |
There was a problem hiding this comment.
What do you think about always populating the discriminant field? (As discussed elsewhere, all variants have one, it's just not exposed for non C-like enums.)
Would make this discriminant: u64.
There was a problem hiding this comment.
Possibly. If we do we should do it as part of #80 though.
| .variant(Variants::new().variant("None", |v| v).variant("Some", |v| { | ||
| v.fields(Fields::unnamed().field(|f| f.ty::<T>())) | ||
| })) |
There was a problem hiding this comment.
rustfmt is doing a terrible job here. :/
|
|
||
| /// Returns the documentation of the variant. | ||
| pub fn docs(&self) -> &[T::String] { | ||
| &self.docs |
There was a problem hiding this comment.
Maybe doc should be an Option. Feels a bit more rusty to check for None rather than an empty slice?
There was a problem hiding this comment.
I prefer an empty slice, it conveys the same meaning and is easier to consume.
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
The main concern I have is how the api becomes a bit clunky to use with this, all the Fn(FieldBuilder) makes things a bit hard to read. I'm not sure if it's a good idea or not but maybe we could have convenience methods like e.g.
pub fn field_of<T: TypeInfo + 'static>(self, name: &'static str, type_name: &'static str) -> Self {
self.field(|f| f.ty::<T>().name(name).type_name(type_name))
}…to cover the common cases? Maybe it's a terrible idea.
There was a problem hiding this comment.
Could add those for use in tests and handrolled impls, but the primary consumer of this API is the derive macro which would always use the builder pattern.
This PR did originally just add the docs to those helper methods, but has changed to the builder pattern after suggestions in this review. I agree it is slightly more clunky but also more flexible and doesn't require all the permutations of the helper methods, especially when we add optional indices in #80.
The main concern I have is how the api becomes a bit clunky to use with this, all the Fn(FieldBuilder)
The original way I tried was to just take am initialized FieldBuilder as an argument instead of the callback, e.g. as we do with Variants::new() etc, but decided it was slightly too verbose. Any other ideas much appreciated - I found this very hard to get just right.
There was a problem hiding this comment.
Yeah, I struggled with this too. I have no better suggestion atm. :/
Definitely agree on the flexibility. We can add the convenience methods if/when we feel we need them.
Co-authored-by: David <[email protected]>
Co-authored-by: David <[email protected]>
The derive macro captures doc comments for types, fields (named and unnamed) and enum variants.
Since docs are optional, builders are introduced for fields and variants. This lays the groundwork for #80 which allows an optional index to be applied to a variant.
Related to paritytech/substrate#8615, This PR will enable removing some custom metadata types in substrate e.g.
FunctionMetadata.todo