Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

#[derive(MaxEncodedLen)]#8737

Merged
6 commits merged intomasterfrom
prgn-derive-maxencodedlen
May 7, 2021
Merged

#[derive(MaxEncodedLen)]#8737
6 commits merged intomasterfrom
prgn-derive-maxencodedlen

Conversation

@coriolinus
Copy link
Contributor

Part of #8719.

@coriolinus coriolinus added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 5, 2021
@coriolinus coriolinus self-assigned this May 5, 2021
@coriolinus coriolinus mentioned this pull request May 5, 2021
20 tasks
@coriolinus coriolinus marked this pull request as ready for review May 5, 2021 14:17
@coriolinus coriolinus requested review from bkchr and gui1117 May 5, 2021 14:17
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 5, 2021
@coriolinus coriolinus requested a review from kianenigma May 5, 2021 14:21
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Just some minor notes, otherwise looks very good.

Comment on lines +85 to +86
// This looks like an overlapping import/export, but it isn't:
// macros and traits live in distinct namespaces.
Copy link
Member

Choose a reason for hiding this comment

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

TIL

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Looks good besides the docs that need to be moved and fixed

/// ```
///
/// ```ignore
/// # // This example is ignored because testing it properly requires imports which would be
Copy link
Member

Choose a reason for hiding this comment

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

As with all other macros. Please move the docs to the re-export in frame-support. There you can make this example compile.

Ingnored examples are rather useless as they can break relative fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok: f15e847.

@coriolinus
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented May 7, 2021

Waiting for commit status.

@ghost ghost merged commit a1eaece into master May 7, 2021
@ghost ghost deleted the prgn-derive-maxencodedlen branch May 7, 2021 08:18
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jun 1, 2021
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
* impl #[derive(MaxEncodedLen)] for structs

* impl #[derive(MaxEncodedLen)] for enums, unions

* break long comments onto multiple lines

* add doc for public item

* add examples to macro documentation

* move MaxEncodedLen macro docs, un-ignore doc-tests
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants