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

Result<Option<>> rather than Option<Option<>> #9119

Merged
merged 2 commits into from
Jun 23, 2021

Conversation

gilescope
Copy link
Contributor

@gilescope gilescope commented Jun 15, 2021

Meme driven development: You are using a three state enum rather than Option<Option<>> right? right???

Edit: No, we're going to use Result<Option<>>

@gilescope gilescope added the A0-please_review Pull request needs code review. label Jun 15, 2021
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Seems like a good idea, but needs a bit more work before it's ready.

primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
@gilescope
Copy link
Contributor Author

(I came across an ICE when coding this - but rust-lang/rust#86368 fixes it and should be in nightly soon)

@gilescope gilescope changed the title Introduced enums to not use Option<Option<>> WIP: Introduced enums to not use Option<Option<>> Jun 18, 2021
@gilescope gilescope changed the title WIP: Introduced enums to not use Option<Option<>> Result<Option<>> rather than Option<Option<>> Jun 18, 2021
@shawntabrizi shawntabrizi added 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 labels Jun 18, 2021
@gilescope gilescope added B7-runtimenoteworthy D2-breaksapi and removed B0-silent Changes should not be mentioned in any release notes D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 18, 2021
@shawntabrizi
Copy link
Member

@gilescope what public runtime APIs does this break?

@gilescope gilescope added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Jun 18, 2021
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
primitives/runtime/src/offchain/storage.rs Outdated Show resolved Hide resolved
@gilescope
Copy link
Contributor Author

@shawntabrizi in primitives/runtime/src/offchain/storage.rs these are the breaking changes:

StorageValueRef::get returns Result<Option<T>> rather than Option<Option<T>>.
StorageValueRef::mutate returns Result<T, MutateStorageError> rathern than a Result<Result<T>> (and also the closure it accepts now takes a Result<Option<T>> rather than an Option<Option<T>>.

They make the api easier to understand.

@gilescope
Copy link
Contributor Author

Hmm, think I need to rebase.

@gilescope
Copy link
Contributor Author

@bkchr can I get an approve from you and can I get an A6-mustntgrumble from you @coriolinus ?
(Had to bump the lock file to make the build happy, but that should be fine as it won't compile with any earlier version - trie-db really should have bumped the middle digit if it was following semver as they added a function).

@gilescope gilescope merged commit 01ff4ce into paritytech:master Jun 23, 2021
athei pushed a commit that referenced this pull request Jun 25, 2021
@redzsina redzsina 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 Jul 13, 2021
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. 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 D2-breaksapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants