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.

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

refactor(trie): rename persistRoot to useRootPersistence #2223

Merged
merged 37 commits into from
Aug 24, 2022

Conversation

faustbrian
Copy link
Contributor

@faustbrian faustbrian commented Aug 24, 2022

Leaving as draft until #2215 is merged

Bringing this property in line with the naming of other functional modifiers. Also see #2226.

holgerd77 and others added 30 commits August 23, 2022 12:32
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
Signed-off-by: Brian Faust <hello@basecode.sh>
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #2223 (1c5fe9f) into master (fffe4ab) will increase coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head 1c5fe9f differs from pull request most recent head 4d14f36. Consider uploading reports for the commit 4d14f36 to get more accurate results

Impacted file tree graph

Flag Coverage Δ
block 92.98% <ø> (ø)
blockchain 88.88% <ø> (?)
client 87.00% <ø> (ø)
common 59.55% <ø> (ø)
devp2p 92.33% <ø> (ø)
ethash ∅ <ø> (∅)
evm 79.07% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 88.15% <ø> (ø)
trie 89.95% <100.00%> (ø)
tx 97.98% <ø> (ø)
util 92.33% <ø> (ø)
vm 85.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@faustbrian faustbrian force-pushed the rename-useRootPersistence branch from ad8971e to 54da8af Compare August 24, 2022 05:27
@faustbrian faustbrian changed the title refactor(trie): rename persistRoot to useRootPersistence refactor(trie): rename persistRoot to usePersistedRoot Aug 24, 2022
@faustbrian faustbrian force-pushed the rename-useRootPersistence branch 2 times, most recently from 48b26a8 to b7fa719 Compare August 24, 2022 06:52
@faustbrian faustbrian changed the title refactor(trie): rename persistRoot to usePersistedRoot refactor(trie): rename persistRoot to useRootPersistence Aug 24, 2022
@faustbrian faustbrian force-pushed the rename-useRootPersistence branch from b7fa719 to cb3809a Compare August 24, 2022 06:53
@faustbrian faustbrian changed the title refactor(trie): rename persistRoot to useRootPersistence refactor(trie): rename persistRoot to useRootPersistence Aug 24, 2022
Base automatically changed from trie-checkpoint-option to master August 24, 2022 09:46
@faustbrian
Copy link
Contributor Author

@holgerd77 conflicts resolved

@faustbrian faustbrian marked this pull request as ready for review August 24, 2022 09:54
@faustbrian
Copy link
Contributor Author

Just ran this against my app and its test suite and all good.

@faustbrian faustbrian requested a review from holgerd77 August 24, 2022 10:27
@faustbrian
Copy link
Contributor Author

@holgerd77 this is green

@faustbrian
Copy link
Contributor Author

@holgerd77 no conflicts with the checkpoint PR so could admin merge

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

This looks good as well (no approval yet).

@holgerd77
Copy link
Member

Hmm, everything is blocked here. Maybe give this combined-PR-suggestion another thought, I just simply otherwise can't guarantee that we still include these changes in the release since I might not have the time any more to approve one-by-one with an hour or so in-between-wait-time. This is just our CI very very much at its limits, since a single merge is triggering re-runs all over the place, not necessarily the right ones queued first.

I am not seeing so much the revert difficulty problem. All things remain in single commits and if one would run at least the Trie tests after each cherry-pick this should remain under control I guess.

So maybe give this another thought, also important to close the taken-over PRs.

@holgerd77
Copy link
Member

(and things have mostly already proven to be working by CI having passed on close to all (all?) separate PRs)

@faustbrian faustbrian changed the base branch from master to key-hashing-name August 24, 2022 11:33
Base automatically changed from key-hashing-name to big-messy-bunch August 24, 2022 11:35
@faustbrian faustbrian merged commit 74994e1 into big-messy-bunch Aug 24, 2022
@faustbrian faustbrian deleted the rename-useRootPersistence branch August 24, 2022 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants