Handle #[codec(index = …)] in regular enums
#80
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By default, parity-scale-codec does not provide Encode/Decode impls for an owned String. This is only provided under the "full" feature which is not used by the substrate runtime, because it should not be used for consensus critical code. So in order for the CompactForm to be integrated into the substrate runtime, or wherever the "full" feature cannot be used, then we must parameterize the `String` type so that it can be both an `&'static str` on the runtime side where it is encoded, and a `String` in client/consuming code where it is decoded.
Add a `compact` member to `Field` to indicate it is `scale_codec::Compact`
…ncoded/decoded as a SCALE Compact type
Handle `codec(skip)` and `codec(index = $int)` attributes
Ensure we're working with an outer attribute
1 task
Co-authored-by: Robin Freyler <[email protected]>
dvdplm
commented
Jun 5, 2021
| match v.fields { | ||
| let v_name = quote! {::core::stringify!(#ident) }; | ||
| let docs = utils::get_doc_literals(&v.attrs); | ||
| let index = utils::maybe_index(v).map(|i| quote!(.index(#i))); |
There was a problem hiding this comment.
It's hard to overstate how much nicer this is compared to what I had.
There was a problem hiding this comment.
That was one of my goals with the builder pattern changes, so thanks!, The main priorities being type safe construction and friendliness for the derive macro. The tradeoff being that it is not fantastic for constructing type infos by hand, but we are assuming that is not very common.
ascjones
added a commit
that referenced
this pull request
Jun 6, 2021
* Capture docs for types, fields, and enum variants * Add docs to derive tests * Fmt * Fmt * Make clippy happy * Fix struct docs tests * Fix struct docs tests * Add missing Vec import * Fix test * Clear docs * Promote build to it's own mod, split fields and variant builders * Refactor variant construction with builder * Fix up derive and unit tests with Variant builder * Fmt * Condense build module back to single file * Fix up doc tests * Fix up README * Define initial field builder * Fix up field building * Fix tests * Fully qualify calls to stringify * Default impl for FieldBuilder * Fix up default impl * Fix spelling errors * Remove clear_docs * Fully qualify module_path * Update derive/src/lib.rs Co-authored-by: Robin Freyler <[email protected]> * Use callback for variant builder * Update README * Remove leading space in doc comments * Satisfy clippy * Add test for doc capture * Rename index back to discriminant, changes for this will be in #80 * Rename index back to discriminant, changes for this will be in #80 * Update src/build.rs Co-authored-by: David <[email protected]> * Update src/build.rs Co-authored-by: David <[email protected]> Co-authored-by: Robin Freyler <[email protected]> Co-authored-by: David <[email protected]>
ascjones
approved these changes
Jun 6, 2021
| match v.fields { | ||
| let v_name = quote! {::core::stringify!(#ident) }; | ||
| let docs = utils::get_doc_literals(&v.attrs); | ||
| let index = utils::maybe_index(v).map(|i| quote!(.index(#i))); |
There was a problem hiding this comment.
That was one of my goals with the builder pattern changes, so thanks!, The main priorities being type safe construction and friendliness for the derive macro. The tradeoff being that it is not fantastic for constructing type infos by hand, but we are assuming that is not very common.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A Variant can have fields of 3 different kinds:
Named,UnnamedorUnit. In this PR I introduce indexed variations of these, so we haveVariantsBuilder::variant/VariantsBuilder::indexed_variantandVariantsBuilder::variant_unit/VariantsBuilder::indexed_variant_unit.This PR stores the index in a new
indexmember ofVariant, which is probably a bit contentious given it increases the size of the registry.We could also store the index in the
discriminantfield, which would be compatible with the direction the Rust language is heading with RFC 2363 (surfacing discriminants for all enum variants, c-style or not).It is currently possible to set both the discriminant and the index on a c-style enum (with priority given to the index attribute):
I think that the more cautious path is to have both
indexanddiscriminant, mimicingparity-scale-infos behaviour.Builds on top of #44
Closes #79