Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capture doc comments, add variant and field builders #87

Merged
merged 38 commits into from
Jun 6, 2021
Merged

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Apr 26, 2021

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

  • fix tests

src/registry.rs Outdated Show resolved Hide resolved
src/ty/variant.rs Outdated Show resolved Hide resolved
test_suite/tests/derive.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm; the only outstanding issues as far as I can see are:

  • whether we want to refactor the crate to use a more extensive builder-pattern. I had a stab at that a while back and found it tricky.
  • a few more tests for the #[doc] attribute

src/registry.rs Outdated Show resolved Hide resolved
src/registry.rs Outdated Show resolved Hide resolved
test_suite/tests/derive.rs Outdated Show resolved Hide resolved
test_suite/tests/derive.rs Outdated Show resolved Hide resolved
test_suite/tests/derive.rs Outdated Show resolved Hide resolved
test_suite/tests/derive.rs Show resolved Hide resolved
derive/src/lib.rs Outdated Show resolved Hide resolved
@ascjones ascjones changed the title Capture doc comments Capture doc comments, add variant and field builders May 26, 2021
@ascjones ascjones mentioned this pull request May 26, 2021
derive/src/lib.rs Outdated Show resolved Hide resolved
derive/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/build.rs Outdated Show resolved Hide resolved
pub struct VariantBuilder {
name: &'static str,
fields: Vec<Field<MetaForm>>,
discriminant: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. If we do we should do it as part of #80 though.

src/build.rs Outdated Show resolved Hide resolved
Comment on lines +139 to +141
.variant(Variants::new().variant("None", |v| v).variant("Some", |v| {
v.fields(Fields::unnamed().field(|f| f.ty::<T>()))
}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfmt is doing a terrible job here. :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed 😞


/// Returns the documentation of the variant.
pub fn docs(&self) -> &[T::String] {
&self.docs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe doc should be an Option. Feels a bit more rusty to check for None rather than an empty slice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer an empty slice, it conveys the same meaning and is easier to consume.

self
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ascjones ascjones May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ascjones and others added 2 commits May 27, 2021 14:38
Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: David <dvdplm@gmail.com>
@ascjones ascjones merged commit 700f810 into master Jun 6, 2021
@ascjones ascjones deleted the aj-docs branch June 6, 2021 15:19
@ascjones ascjones mentioned this pull request Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants