paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Restructure `frame_support` macro related exports #14745

Closed juangirini closed 1 year ago

juangirini commented 1 year ago

Partially for https://github.com/paritytech/polkadot-sdk/issues/172

juangirini commented 1 year ago

bot fmt

command-bot[bot] commented 1 year ago

@juangirini https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3379775 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 30-ab51b0e6-b330-4de7-a7f0-3557e9051f13 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot[bot] commented 1 year ago

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

ggwpez commented 1 year ago

It looks like we can first merge the companions, since they dont use the new __private module but are just normal refactors (as it should be).

gilescope commented 1 year ago

This is going to lead to quite a bit of downstream breaking changes. I seem to recall @kianenigma was suggesting that we hide a lot of common imports under one crate ( https://github.com/paritytech/substrate/pull/14137 ) - this seems like going in the opposite direction? If we're going to do both then it would be kinder for downstream if we landed both PRs at the same time as downstream would probably then have less work to update their codebases.

juangirini commented 1 year ago

@bkchr @ggwpez I wanted to keep this PR bounded to the frame_support reexports only, I have updated the PR title accordingly. I will address other modules in follow up PRs

juangirini commented 1 year ago

This is going to lead to quite a bit of downstream breaking changes. I seem to recall @kianenigma was suggesting that we hide a lot of common imports under one crate - this seems like going in the opposite direction? If we're going to do both then it would be kinder for downstream if we landed both PRs at the same time as downstream would probably then have less work to update their codebases.

Unfortunately, as mentioned in https://github.com/paritytech/substrate/issues/14501 we can't 'kindly' deprecate re-exports, so yes there will be many breaking changes (you can already see that in the companion PRs). Regarding Kian's crate, I think you mean the "umbrella crate" (https://github.com/paritytech/substrate/pull/14137). Definitely this effort and his PR will have some conflicts to resolve, but I don't see how the umbrella crate is not compatible with this.

bkchr commented 1 year ago

@bkchr @ggwpez I wanted to keep this PR bounded to the frame_support reexports only, I have updated the PR title accordingly. I will address other modules in follow up PRs

Yeah that is fine, but the things I highlighted is where you replaced frame support with an export from another crate that you should not use. Just make it correct directly. I don't mean to change the export, but to use the correct import.

juangirini commented 1 year ago

bot fmt

command-bot[bot] commented 1 year ago

@juangirini https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3401090 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 37-5bad9448-a260-4c19-941b-b85f3335db38 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot[bot] commented 1 year ago

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

ggwpez commented 1 year ago

This is going to lead to quite a bit of downstream breaking changes. I seem to recall @kianenigma was suggesting that we hide a lot of common imports under one crate ( paritytech/substrate#14137 ) - this seems like going in the opposite direction? If we're going to do both then it would be kinder for downstream if we landed both PRs at the same time as downstream would probably then have less work to update their codebases.

@gilescope it should not break downstream - unless downstream used internal re-exports, which is a crime anyway…
We now just make the fact that they are internal, more explicit.

juangirini commented 1 year ago

bot rebase

paritytech-processbot[bot] commented 1 year ago

Rebased

juangirini commented 1 year ago

bot rebase

paritytech-processbot[bot] commented 1 year ago

Rebased

juangirini commented 1 year ago

bot rebase

paritytech-processbot[bot] commented 1 year ago

Rebased

juangirini commented 1 year ago

bot merge

paritytech-processbot[bot] commented 1 year ago

Waiting for commit status.