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

Pub enum runtime to pub struct runtime #13250

Merged
bkchr merged 10 commits into from
Mar 7, 2023
Merged

Pub enum runtime to pub struct runtime #13250

bkchr merged 10 commits into from
Mar 7, 2023

Conversation

@a-moreira
Copy link
Contributor

@a-moreira a-moreira commented Jan 27, 2023 •
edited
Loading

This PR simply substitutes Pub enum Runtime for Pub struct Runtime in the construct_runtime! macros, as suggested by @kianenigma on the Polkadot Blockchain Academy this week :-)

polkadot address: 14WCnNFaTCYW2BTS2rDMHacvWdfQEPp2kJjAsp9VdHJJrNNT

@ggwpez
Copy link
Member

ggwpez commented Jan 27, 2023

This would be a breaking change for all downstream projects…
@kianenigma sure that this is worth it?

ggwpez
ggwpez reviewed Jan 27, 2023
View reviewed changes
@shawntabrizi
Copy link
Member

shawntabrizi commented Jan 27, 2023 •
edited
Loading

This should be non-breaking and work out of the box afaik.

construct_runtime! already supports this: #11932

So yeah, I think its worth.

@a-moreira please run cargo +nightly fmt

and please add your Polkadot / Kusama address to your PR description per https://github.com/paritytech/substrate-tip-bot

@shawntabrizi shawntabrizi added Z0-trivial Writing the issue is of the same difficulty as patching the code. 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit and removed Z0-trivial Writing the issue is of the same difficulty as patching the code. labels Jan 27, 2023
@ggwpez ggwpez added the A0-please_review Pull request needs code review. label Jan 27, 2023
@kianenigma
Copy link
Contributor

kianenigma commented Jan 28, 2023

This should not be breaking as Shawn said. Just use pub struct Runtime in our codebase, and mark usage of pub enum Runtime as deprecated.

kianenigma
kianenigma approved these changes Jan 28, 2023
View reviewed changes
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.

I'm down to mark the usage of pub enum whatever as deprecated as well, and then remove it.

If so others agree, please do that in this PR as well.

@xlc
Copy link
Contributor

xlc commented Jan 31, 2023

What's the reason for doing this?

xlc
xlc reviewed Jan 31, 2023
View reviewed changes
@shawntabrizi
Copy link
Member

shawntabrizi commented Jan 31, 2023

What's the reason for doing this?

We are at the academy, and it is just more clear to use a struct than an enum for the "runtime" container.

@kianenigma kianenigma requested review from shawntabrizi and xlc and removed request for xlc February 14, 2023 01:26
@kianenigma
Copy link
Contributor

kianenigma commented Feb 14, 2023

bot rebase

@paritytech-processbot
Copy link

paritytech-processbot bot commented Feb 14, 2023

Error: Command 'Command { std: "git" "push" "--porcelain" "a-moreira" "pub-enum-runtime-to-pub-struct-runtime", kill_on_drop: false }' failed with status Some(1); output: error: failed to push some refs to 'https://x-access-token:${SECRET}@github.com/a-moreira/substrate.git'

@kianenigma
Copy link
Contributor

kianenigma commented Feb 14, 2023

CI needs work @a-moreira

@a-moreira
Copy link
Contributor Author

a-moreira commented Feb 20, 2023 •
edited
Loading

@kianenigma sorry for the delay, I was AFK in the last days. Fixing now

@gilescope gilescope mentioned this pull request Mar 6, 2023
Closed
@a-moreira
Copy link
Contributor Author

a-moreira commented Mar 7, 2023 •
edited
Loading

@kianenigma CI works. I updated trybuild error files (like this approach #8372) and I created another file to keep a test for Pub enum Runtime. no answers, so assuming it would be better to deprecate the other stuff in another PR.

@bkchr bkchr requested a review from a team March 7, 2023 09:16
@bkchr
Copy link
Member

bkchr commented Mar 7, 2023

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented Mar 7, 2023

Waiting for commit status.

@paritytech-processbot
Copy link

paritytech-processbot bot commented Mar 7, 2023

Merge cancelled due to error. Error: Statuses failed for 2daecc0

@bkchr
Copy link
Member

bkchr commented Mar 7, 2023

bot rebase

@paritytech-processbot
Copy link

paritytech-processbot bot commented Mar 7, 2023

Rebased

@bkchr
Copy link
Member

bkchr commented Mar 7, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Mar 7, 2023 •
edited
Loading

@bkchr https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2493661 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 27-daa069ac-ae78-4d7f-86f4-887aa687abae to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 7, 2023

@bkchr Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2493661 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2493661/artifacts/download.

@bkchr bkchr merged commit 8d0b756 into paritytech:master Mar 7, 2023
@Polkadot-Forum
Copy link

Polkadot-Forum commented Mar 30, 2023

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-40/2468/1

@nazar-pc nazar-pc mentioned this pull request Apr 2, 2023
Merged
1 task
@kiltbot kiltbot mentioned this pull request Apr 3, 2023
Closed
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
@a-moreira @bkchr @ukint-vs
Pub enum runtime to pub struct runtime (paritytech#13250)
a46dc41 
* pub enum Runtime -> pub struct Runtime

* changing some more

* fmt

* updating *.stderr files

* re-run trybuild after rust update

* keep a test file for `pub enum Runtime`

* Delete construct_runtime_2.rs

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: parity-processbot <>
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@a-moreira @bkchr @nathanwhit
Pub enum runtime to pub struct runtime (paritytech#13250)
2b437c9 
* pub enum Runtime -> pub struct Runtime

* changing some more

* fmt

* updating *.stderr files

* re-run trybuild after rust update

* keep a test file for `pub enum Runtime`

* Delete construct_runtime_2.rs

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: parity-processbot <>
@Marcin-Radecki Marcin-Radecki mentioned this pull request Sep 18, 2023
Merged
@dmitrylavrenov dmitrylavrenov mentioned this pull request Nov 28, 2023
Closed
46 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@shawntabrizi shawntabrizi Awaiting requested review from shawntabrizi

4 more reviewers

@xlc xlc xlc left review comments

@ggwpez ggwpez ggwpez left review comments

@kianenigma kianenigma kianenigma approved these changes

@bkchr bkchr bkchr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

Runtime / FRAME
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@a-moreira @ggwpez @shawntabrizi @kianenigma @xlc @bkchr @Polkadot-Forum