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.

Sign up for GitHub

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

Jump to bottom

Replace unnecessary &mut self with &self in BlockImport::import_block() #5339

Merged
merged 4 commits into from
Aug 18, 2024
Merged

Replace unnecessary &mut self with &self in BlockImport::import_block() #5339

merged 4 commits into from
Aug 18, 2024

Conversation

nazar-pc
Copy link
Contributor

There was no need for it to be &mut self since block import can happen concurrently for different blocks and in many cases it was &mut Arc<dyn BlockImport> anyway 🤷‍♂️

Similar in nature to #4844

@nazar-pc nazar-pc requested review from sorpaas and a team as code owners August 13, 2024 10:10
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 3/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6984469

@nazar-pc nazar-pc force-pushed the no-mut-block-import branch from 1238396 to 90f6ec0 Compare August 13, 2024 11:07
@nazar-pc
Copy link
Contributor Author

Is there some kind of tooling for semver? There are a lot of files changed and I'd hate to go through each crate manually.

@ggwpez
Copy link
Member

ggwpez commented Aug 13, 2024

Is there some kind of tooling for semver? There are a lot of files changed and I'd hate to go through each crate manually.

Yea am gonna try to run the bot now for this. Just added it now #5331

@github-actions GitHub Actions
Copy link

Command failed ❌

Run by @ggwpez for Command PrDoc failed. See logs here.

@paritytech paritytech deleted a comment from github-actions bot Aug 13, 2024
@ggwpez
Copy link
Member

ggwpez commented Aug 13, 2024

Looks like the bot failed. Ran the script manually now. You probably want to demote some of these majors to patch if they are not publicly exposed. This prdoc is necessary as to not break SemVer for parachains.

Details 

crates:
- bump: major
  name: cumulus-client-consensus-common
- bump: major
  name: cumulus-client-network
- bump: major
  name: cumulus-relay-chain-inprocess-interface
- bump: major
  name: cumulus-pallet-parachain-system
- bump: major
  name: sc-basic-authorship
- bump: major
  name: sc-consensus-babe
- bump: major
  name: sc-consensus-beefy
- bump: major
  name: sc-consensus
- bump: major
  name: sc-consensus-grandpa
- bump: major
  name: sc-consensus-pow
- bump: major
  name: mmr-gadget
- bump: major
  name: sc-network
- bump: major
  name: sc-network-sync
- bump: major
  name: sc-offchain
- bump: major
  name: sc-rpc-spec-v2
- bump: major
  name: sc-rpc
- bump: major
  name: sc-service
- bump: major
  name: sc-transaction-pool
doc:
- audience: Node Dev
  description: "There was no need for it to be `&mut self` since block import can\
    \ happen concurrently for different blocks and in many cases it was `&mut Arc<dyn\
    \ BlockImport>` anyway :man_shrugging:\r\n\r\nSimilar in nature to https://github.com/paritytech/polkadot-sdk/pull/4844"
title: "Replace unnecessary `&mut self` with `&self` in `BlockImport::import_block()`"

@nazar-pc
Copy link
Contributor Author

Nice, thank you!

@ggwpez ggwpez mentioned this pull request Aug 14, 2024
Merged
bkchr
bkchr approved these changes Aug 14, 2024
View reviewed changes
substrate/test-utils/client/src/client_ext.rs Outdated Show resolved Hide resolved
substrate/test-utils/client/src/client_ext.rs Outdated Show resolved Hide resolved
@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Aug 14, 2024
@bkchr bkchr requested a review from a team August 14, 2024 20:11
@davxy davxy added this pull request to the merge queue Aug 18, 2024
Merged via the queue into paritytech:master with commit feac7a5 Aug 18, 2024
173 of 176 checks passed
@nazar-pc nazar-pc deleted the no-mut-block-import branch August 19, 2024 15:26
programskillforverification pushed a commit to programskillforverification/polkadot-sdk that referenced this pull request Aug 19, 2024
@nazar-pc @programskillforverification
Replace unnecessary &mut self with &self in `BlockImport::import_…
2ce6458 
…block()` (paritytech#5339)

There was no need for it to be `&mut self` since block import can happen
concurrently for different blocks and in many cases it was `&mut Arc<dyn
BlockImport>` anyway 🤷‍♂️

Similar in nature to
paritytech#4844
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2024
@ggwpez
[CI] Fix SemVer check base commit (#5361)
b34d4a0 
After seeing some cases of reported changes that did not happen by the
merge request proposer (like
#5339), it became clear
that [this](https://github.com/orgs/community/discussions/59677) is
probably the issue.
The base commit of the SemVer check CI is currently using the *latest*
master commit, instead of the master commit at the time when the MR was
created.

Trying to get the correct base commit now. For this to be debugged, i
have to wait until another MR is merged into master.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez mentioned this pull request Sep 9, 2024
Closed
@github-actions github-actions bot mentioned this pull request Oct 8, 2024
Closed
@TDemeco TDemeco mentioned this pull request Oct 14, 2024
Merged
@github-actions github-actions bot mentioned this pull request Dec 5, 2024
Open
@ToufeeqP ToufeeqP mentioned this pull request Dec 10, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davxy davxy davxy approved these changes

@bkchr bkchr bkchr approved these changes

@skunert skunert skunert approved these changes

@sorpaas sorpaas Awaiting requested review from sorpaas sorpaas is a code owner

Assignees
No one assigned
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nazar-pc @paritytech-cicd-pr @ggwpez @skunert @bkchr @davxy