[fix docs compiler warnings] Election Provider Multi-phase pallet #14645
Conversation
sacha-l
added
A0-please_review
| /// The `MigrateToV1` struct implements the `OnRuntimeUpgrade` trait, which is called | ||
| /// during runtime upgrades. When the runtime is upgraded and this migration is executed, | ||
| /// it checks the current and on-chain storage versions to determine if the migration is | ||
| /// needed. | ||
| /// | ||
| /// If the current storage version is 1 and the on-chain version is 0, the migration will | ||
| /// proceed. During migration, the module checks if the `SignedSubmissionIndices` storage | ||
| /// item exists. If it does, the module migrates its data to the new storage format. |
There was a problem hiding this comment.
This part is very generic to almost every migration out there. Should we instead just point to some resource in the module level doc (mod migrations) and keep mod v1 docs concise?
There was a problem hiding this comment.
Where are you suggesting we move the generic migration docs to ?
There was a problem hiding this comment.
Potentially here (if you feel it needs more explanation)? There is also a storage migration tutorial which hopefully is easy to find.
Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
| /// In this migration, the storage value type `SubmissionIndicesOf` for the storage item | ||
| /// `SignedSubmissionIndices` is changing from `BoundedBTreeMap<ElectionScore, u32, | ||
| /// SignedMaxSubmissions>` to `BoundedVec<(ElectionScore, BlockNumber, u32), SignedMaxSubmissions>` | ||
| /// to allow multiple submissions of same election score to be submitted. See /~https://github.com/paritytech/substrate/pull/14645 for more details about this update. |
| /// room for this one. | ||
| /// If `origin` is `Some(AccountId)`, the stored solution was submitted in the signed phase | ||
| /// by a miner with the `AccountId`. Otherwise, the solution was stored either during the | ||
| /// unsigned phase or by `T::ForceOrigin`. |
There was a problem hiding this comment.
| /// unsigned phase or by `T::ForceOrigin`. | |
| /// unsigned phase or by `[T::Config::ForceOrigin]`. |
I have seen that in all your doc PRs you are not really doing the linking correctly, please be more careful with that.
There was a problem hiding this comment.
| // This is only useful for a context where a `<T: Config>` is not in scope. | ||
| /// Macro to log messages without including the system block number in the log. | ||
| /// | ||
| /// This macro is a logging utility used within the crate. It allows logging messages without |
There was a problem hiding this comment.
Please keep the principle of remaining concise and DRY in mind.
This whole block should be:
/// Same as [`log`], but in cases where a `T` implementing [`Config`] is not in scope.
| @@ -15,6 +15,46 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| //! Storage migrations for the Election Provider Multi-phase pallet. | |||
There was a problem hiding this comment.
All of these statements are more or less about what migrations are and in a generic sense.
If I take out Election Provider Multi-phase our of this block and replace it with Any Other Pallet all of this still holds.
This means you are documenting the wrong thing here. You are documenting what a runtime upgrade in general is, NOT what the runtime of this pallet is doing. This generic information about runtime upgrades in general, if they should be anywhere, should be on the OnRuntimeUpgrade or Hooks traits. Or, in an external website or elsewhere. Then, you link to that external resource here, rather than potentially copy-pasting this everywhere.
There was a problem hiding this comment.
I am happy that you and @aaronbassett are giving this a shot, but
- I don't see /~https://github.com/paritytech/substrate/blob/master/docs/DOCUMENTATION_GUIDELINES.md and more importantly the 5 principles mentioned https://forum.parity.io/t/new-polkadot-substrate-documentation-plan/1810 being respected here.
- I do want to warn you that no documentation is strictly better than wrong documentation. As you are getting closer and closer to domain specific extensive pallets (such as this one), please consult the correct owner (= whoever knows best about the pallet) and try and work with them to write this, rather than taking it all on yourself. Example, I think you having requested review from @Ank4n and @gpestana is the def. the right thing to do 👍
This PR fixes the 26 compiler warnings emitted by running
RUSTFLAGS="-W missing_docs" cargo check -p pallet-election-provider-multi-phaseby adding missing documentation throughout the crate.