Skip to content

Renames WeightMeter constructors for better readability#1186

Merged
gupnik merged 5 commits intomasterfrom
gupnik/weight-meter-update
Sep 1, 2023
Merged

Renames WeightMeter constructors for better readability#1186
gupnik merged 5 commits intomasterfrom
gupnik/weight-meter-update

Conversation

@gupnik
Copy link
Copy Markdown
Contributor

@gupnik gupnik commented Aug 26, 2023

As discussed in the Elements chat, this PR adds the following changes:

  1. Renames WeightMeter::max_limit() to WeightMeter::new() as the maximum weight limit is implied
  2. Renames WeightMeter::from_limit() to WeightMeter::with_limit()

@gupnik gupnik added A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). T1-FRAME This PR/Issue is related to core FRAME, the framework. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Aug 26, 2023
@gupnik gupnik requested a review from ggwpez August 26, 2023 06:00
@paritytech-ci paritytech-ci requested review from a team August 26, 2023 06:00
/// Creates [`Self`] with the maximal possible limit for the consumable weight.
pub fn max_limit() -> Self {
Self::from_limit(Weight::MAX)
pub fn new() -> Self {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But the old one was more expressive?

Copy link
Copy Markdown
Contributor Author

@gupnik gupnik Aug 26, 2023

Choose a reason for hiding this comment

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

This comes after a discussion with @gavofyork around the readability of this piece of code:

let meter = WeightMeter::max_limit();
meter.consume(some_weight);

It reads as though the meter starts from the limit and should saturate with the further consume call. Going through the impl makes it clear that it still starts from 0.

Since the limit to max is implied with a meter, we should not need to specify it explicitly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe with_max_limit? I mean your example is understandable, but new is still not really expressive to what the limit will be. (I will also not block on this, just some thought)

@paritytech-ci paritytech-ci requested review from a team August 26, 2023 16:06
/// Creates [`Self`] with the maximal possible limit for the consumable weight.
pub fn max_limit() -> Self {
Self::from_limit(Weight::MAX)
pub fn new() -> Self {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe with_max_limit? I mean your example is understandable, but new is still not really expressive to what the limit will be. (I will also not block on this, just some thought)

@paritytech-ci paritytech-ci requested a review from a team August 28, 2023 09:35
Copy link
Copy Markdown
Contributor

@ordian ordian left a comment

Choose a reason for hiding this comment

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

(approve to unblock the merge)

@gupnik gupnik merged commit 16fe5be into master Sep 1, 2023
@gupnik gupnik deleted the gupnik/weight-meter-update branch September 1, 2023 06:46
ordian added a commit that referenced this pull request Sep 1, 2023
* master: (25 commits)
  fix typos (#1339)
  Use bandersnatch-vrfs with locked dependencies ref (#1342)
  Bump bs58 from 0.4.0 to 0.5.0 (#1293)
  Contracts: `seal0::balance` should return the free balance (#1254)
  Logs: add extra debug log for negative rep changes (#1205)
  Added short-benchmarks for cumulus (#1183)
  [xcm-emulator] Improve hygiene and clean up (#1301)
  Bump the known_good_semver group with 1 update (#1347)
  Renames API (#1186)
  Rename `polkadot-parachain` to `polkadot-parachain-primitives` (#1334)
  Add README to project root (#1253)
  Add environmental variable to track decoded instructions (#1320)
  Fix polkadot-node-core-pvf-prepare-worker build with jemalloc (#1315)
  Sassafras primitives (#1249)
  Restructure `dispatch` macro related exports (#1162)
  backing: move the min votes threshold to the runtime (#1200)
  Bump zstd from 0.11.2+zstd.1.5.2 to 0.12.4 (#1326)
  Remove `substrate_test_utils::test` (#1321)
  remove disable-runtime-api (#1328)
  [ci] add more jobs for pipeline cancel, cleanup (#1314)
  ...
Daanvdplas pushed a commit that referenced this pull request Sep 11, 2023
Co-authored-by: Javier Viola <[email protected]>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* change const value visable

* Add docs

* Update docs

* Update docs 2

* Fix ci

* Fix spell check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A1-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. 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.

6 participants