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

MSA Pallet Docs #1962

Merged
merged 6 commits into from
May 8, 2024
Merged

MSA Pallet Docs #1962

merged 6 commits into from
May 8, 2024

Conversation

wilwade
Copy link
Collaborator

@wilwade wilwade commented May 6, 2024

Goal

The goal of this PR is to improve the documentation of the Pallets and make that documentation be able to be used on docs.frequency.xyz.

Part of frequency-chain/docs#59

Discussion

  • This is the first one, so it includes the needed documentation and templates as well.
  • Please suggest additional content that would be useful as well
  • Had a hard time getting it to look nice in both markdown and the rendered html.
  • Disabling soft wrap will help it look better
  • The idea was to keep rust specific things out of the readme and in the lib.rs

Testing

  • make docs
  • open target/doc/pallet_msa/index.html

Screenshots

image image

@@ -197,7 +197,7 @@ benchmarks-capacity:

.PHONY: docs
docs:
RUSTDOCFLAGS="--enable-index-page -Zunstable-options" cargo doc --no-deps --features frequency
RUSTC_BOOTSTRAP=1 RUSTDOCFLAGS="--enable-index-page -Zunstable-options" cargo doc --no-deps --features frequency
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Needed for some versions of rust, but fine on all as far as I can tell

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Try reviewing in "Rich diff"
image

@@ -1,35 +1,13 @@
//! # MSA Pallet
//! The MSA pallet provides functionality for handling Message Source Accounts.
//! Message Source Account Management
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is here as it is what is picked up by rust docs to show as the short description in the docs search

@@ -0,0 +1,16 @@
#!/bin/sh

# Quick script to output the runtime spec_version at a particular commit.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Helpful for getting the spec_version in the past. I can drop this file if people think it isn't worth it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

Neat

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
pallets/msa/src/lib.rs 87.90% <100.00%> (ø)

pallets/msa/README.md Outdated Show resolved Hide resolved
pallets/msa/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

A few suggestions, but non-blocking

@jeanettedepatie
Copy link
Collaborator

In my view, the Extrinsics table is cut off on the bottom.

Copy link
Collaborator

@jeanettedepatie jeanettedepatie left a comment

Choose a reason for hiding this comment

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

Also, the links going to the rust docs don't seem to work for me. Not sure if I'm improperly configured. Otherwise looks good.


| Name/Description | Caller | Payment | Key Events | Runtime Added |
| --------------------------------------------------------------------------------------------- | ---------------------------------------- | ------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------- |
| `add_public_key_to_msa`<br />Add MSA control key | Delegator or Provider with Signature | Free | [`PublicKeyAdded`](https://frequency-chain.github.io/frequency/pallet_msa/pallet/enum.Event.html#variant.PublicKeyAdded) | 1 |
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Depending on the audience, is Free ambiguous? e.g. Free for end users, who are using Capacity. Not sure if this is a good spot to make the distinction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually this line is wrong! This one is supposed to be Capacity which is separate from Free. The Caller column was supposed to help disambiguate Capacity and free.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed! Does it work better now?

Copy link
Collaborator

@mattheworris mattheworris left a comment

Choose a reason for hiding this comment

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

  • One non-blocking comment
  • Nice improvements

🚢 it!

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

looks great, thanks!

  • I reviewed the code
  • I read the markdown
  • I generated the docs and looked at some of the pages.

@wilwade wilwade merged commit a3abf5b into main May 8, 2024
30 checks passed
@wilwade wilwade deleted the docs/pallet-docs branch May 8, 2024 15:33
shannonwells pushed a commit that referenced this pull request May 16, 2024
# Goal
The goal of this PR is to improve the documentation of the Pallets and
make that documentation be able to be used on `docs.frequency.xyz`.

Part of frequency-chain/docs#59

# Discussion
- This is the first one, so it includes the needed documentation and
templates as well.
- Please suggest additional content that would be useful as well
- Had a hard time getting it to look nice in both markdown and the
rendered html.
- Disabling soft wrap will help it look better
- The idea was to keep rust specific things out of the readme and in the
`lib.rs`

# Testing
- `make docs`
- `open target/doc/pallet_msa/index.html`

Co-authored-by: Joe Caputo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants