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

Add custom error message for StorageNoopGuard #1727

Merged
merged 13 commits into from
Sep 27, 2023

Conversation

seadanda
Copy link
Contributor

Expand StorageNoopGuard to be able to add extra context through a custom error message. When the guard is triggered it panics with an error message which can be defaulted, set on construction, or set after it has been constructed.

Turn StorageNoopGuard into struct with storage_root and error_message and added from_error_message constructor and set_error_message setter.

Also added new() aliased to default().

Closes #375

Turn StorageNoopGuard into struct with storage_root and error_message.
Add from_error_message constructor.
Add set_error_message setter.
@seadanda seadanda added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Sep 27, 2023
@seadanda seadanda requested review from a team September 27, 2023 11:58
@seadanda seadanda self-assigned this Sep 27, 2023
@seadanda seadanda requested a review from ggwpez September 27, 2023 11:59
@paritytech-ci paritytech-ci requested a review from a team September 27, 2023 12:56
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Looking good, left some comments.

Also, I think we should also add something like

impl From<&'static str> for StorageNoopGuard {
    fn from(s: &'static str) -> Self {
        StorageNoopGuard {
            storage_root: sp_std::vec::Vec::new(),
            error_message: s,
        }
    }
}

@paritytech-ci paritytech-ci requested a review from a team September 27, 2023 13:07
Co-authored-by: joe petrowski <[email protected]>
@seadanda
Copy link
Contributor Author

Looking good, left some comments.

Also, I think we should also add something like

impl From<&'static str> for StorageNoopGuard {
    fn from(s: &'static str) -> Self {
        StorageNoopGuard {
            storage_root: sp_std::vec::Vec::new(),
            error_message: s,
        }
    }
}

Great, seems like a nice addition. I'll add it in.

liamaharon
liamaharon previously approved these changes Sep 27, 2023
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

LGTM once impl From<&'static str> for StorageNoopGuard is added

Since this is for testing it doesn't need to be optimised unnecessarily
@seadanda
Copy link
Contributor Author

OK to summarise for all reviewers:

  • I have made all suggested improvements to the doc comments and error messages (and thus updated the tests)
  • In the end I've removed the From implementation as it is very close to from_error_message and @ggwpez pointed out that conceptually it is different from how From is normally used.
  • I've switched the type to String now that I understand that it is purely for testing and I don't need to unnecessarily optimise, especially with the benefits that String brings.

I hope everybody is happy with the changes I've gone with. Thanks for the very helpful reviews :)

@ggwpez ggwpez enabled auto-merge (squash) September 27, 2023 14:11
@seadanda seadanda disabled auto-merge September 27, 2023 14:25
@paritytech-cicd-pr
Copy link

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

@paritytech-cicd-pr
Copy link

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

@seadanda seadanda enabled auto-merge (squash) September 27, 2023 15:07
@seadanda seadanda merged commit 02284a3 into master Sep 27, 2023
6 checks passed
@seadanda seadanda deleted the donal-improve-storage-noop-guard branch September 27, 2023 16:39
ordian added a commit that referenced this pull request Oct 10, 2023
* tsv-disabling-node-side: (69 commits)
  runtime-api: cleanup after v7 stabilization (#1729)
  Move requests-responses and polling from `ChainSync` to `SyncingEngine` (#1650)
  Add custom error message for `StorageNoopGuard` (#1727)
  Clarify docs
  cargo fmt
  add a CAVEAT comment
  implement disabled_validators correctly
  remove unnecessary hash string (#1722)
  OpenGov in Westend and Rococo (#1177)
  Associated type Hasher for `QueryPreimage`, `StorePreimage` and `Bounded` (#1720)
  Migrate polkadot-primitives to v6 (#1543)
  genesis-builder: implemented for all runtimes (#1492)
  `BlockId` removal: `tx-pool` refactor (#1678)
  Bump directories from 4.0.1 to 5.0.1 (#1656)
  Allow debug_assertions in short-benchmarks CI job (#1711)
  chainHead/storage: Fix storage iteration using the query key (#1665)
  Implement more useful traits in `Slot` type (#1595)
  Make downloads in parallel and give more time to complete (#1699)
  Bump actions/checkout from 4.0.0 to 4.1.0 (#1688)
  contracts: Fix incorrect storage alias in mirgration (#1687)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Expand `StorageNoopGuard` to be able to add extra context through a
custom error message. When the guard is triggered it panics with an
error message which can be defaulted, set on construction, or set after
it has been constructed.

Turn `StorageNoopGuard` into struct with `storage_root` and
`error_message` and added `from_error_message` constructor and
`set_error_message` setter.

Also added `new()` aliased to `default()`.

Closes paritytech#375

---------

Co-authored-by: joe petrowski <[email protected]>
Co-authored-by: Liam Aharon <[email protected]>
bkchr pushed a commit that referenced this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom error message for StorageNoopGuard
6 participants