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 README.md to umbrella #7600

Merged
merged 10 commits into from
Feb 27, 2025
Merged

Add README.md to umbrella #7600

merged 10 commits into from
Feb 27, 2025

Conversation

huntbounty
Copy link
Contributor

Resolves #7536

@huntbounty
Copy link
Contributor Author

Maybe I’m getting ahead of myself on the versioning stuff. 👼

For sprucing up the README 🎀—logos, badges, maybe a few emojis. Could I borrow some from the repo’s README?

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks for the initiative. There will be some more reviewers taking a look, so maybe wait for that and then look at the whole feedback in general before changing things.

I asked again internally, and apparently it is better if we write this as Rustdoc and then export it. The difference is that we can actually compile the example to see that it works.
But we can do that as last step. Writing it as markdown is good for now since it renders in github.

polkadot-sdk = { version = "0.12.0", features = ["frame-support", "frame-system"] }
```

```rust
Copy link
Contributor

Choose a reason for hiding this comment

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

It is great that this is a full e2e working example!

What is missing is the fact that there is no connection made between polkadot-sdk and polkadot-sdk-frame, which is also exported as a part of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know there is a polkadot-sdk-frame. 👼 Do you mean it's recommended to use frame-support and frame-system from the exported polkadot-sdk-frame?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can learn about both here, and I think this should help you better structure this README to encompass both :)

https://blog.kianenigma.com/posts/tech/polkadot-sdk-2024/

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, polkadot-sdk-frame has its own docs, what you should do here is:

  1. highlight what the difference between the two is. polkadot-sdk should reference polkadot-sdk-frame and visa versa, avoiding confusion about one another
  2. The example you have here should use use polkadot_sdk::polkadot_sdk_frame as frame and follow its tracks.

Copy link
Contributor

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

High level question: if we're adding an umbrella README, does it make sense to have it in here (polkadot-sdk-docs - reference_docs) too? Thinking about include_str! it to the umbrella_crate.rs. Some existing parts from umbrella_crate.rs might still be relevant for this README, like Known Issues, so we can add them to the README and replace the umbrella_crate.rs entirely. I think the rust code sections from the README will be compiled when building the docs for polkadot-sdk-docs too, so we ensure they are still compiling over time.

@huntbounty
Copy link
Contributor Author

It doesn't feel right to me to describe a language feature as a known issue. It kind of makes it sound like it’s a problem with polkadot-sdk that can be fixed, when it’s really neither. What do you think about this instead?

// `frame_support` and `frame_system` are declared here
use polkadot_sdk::*;

#[frame_support::pallet(dev_mode)]
pub mod pallet {
    // `frame_support` and `frame_system` are imported in the outer module,
    // but are not automatically inherited here. Need to "re-import" to make
    // them available in the inner module.
    use super::*;

If it's what you want here, I could also include a bit about how polkadot-sdk is generated. Anything else you want to bring to the README?

@iulianbarbu
Copy link
Contributor

Anything else you want to bring to the README?

No, the thing I was thinking is to merge this README and the umbrella_crate.rs docs from polkadot-sdk-docs. The info in umbrella_crate.rs will be mostly similar to part of the README you're writing - in which case we can drop it and just include_str! this README into it. Don't feel like it is useful to have what's currently in umbrella_crate.rs when seeing this README. What do you guys think? @kianenigma @ggwpez

@huntbounty
Copy link
Contributor Author

Now that the generation section is ported over, the README covers almost everything in umbrella_crate.rs and more. I think it’s safe to drop it or link to the README. 👼

Running generate-umbrella.py will remove the whole directory, so we might also need to adjust the generation script, or else the README content will have to live elsewhere. As for syncing with the rustdoc (umbrella/src/lib.rs), since it’s generated by generate-umbrella.py, should the content come directly from the script or from a source it reads. It feels a bit odd to have to modify the generation script to update the README/documentation, so I guess we’re left with just one option?

@huntbounty
Copy link
Contributor Author

Do you think we should mention the known issues ATM, like there’s no way to enable a feature of a feature? Maybe also include workarounds if they exist.

@nkpar
Copy link

nkpar commented Feb 24, 2025

For future reference, @huntbounty, it’s a good practice to close review comments once you’ve addressed them.

LGTM otherwise!

@huntbounty
Copy link
Contributor Author

Thanks for the heads-up! I thought it was the reviewers who should decide whether something is resolved and mark it as such, as I may be wrong in thinking something is fixed when it’s not. 👼

By the way, where do contributors usually hang out, or is there a place I can ask questions unrelated to the PR?


To learn more about building with the Polkadot SDK, you may start with these [guides](https://paritytech.github.io/polkadot-sdk/master/polkadot_sdk_docs/guides/index.html) and our [official docs](https://docs.polkadot.com/).

## 💡 Pro Tips
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also put this into the drop-down spoiler from above. Github supports <details> for that.


We do a stable release for the SDK every three months with a version schema reflecting the release cadence, which is tracked in the [release registry](https://github.com/paritytech/release-registry/). At the time of writing, the latest version is `stable2412` (released in 2024 December). To avoid confusion, we will align the versioning of `polkadot-sdk` with the established schema. For instance, the next stable version will be `2503.0.0`.

## 📝 Note to Contributors
Copy link
Member

Choose a reason for hiding this comment

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

I think these could go into a MAINTAIN.md, its not a convention in our repo yet, but this info does not really pertain to the average reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about putting this info somewhere else like ./CONTRIBUTING.md instead of in this crate? As it is supposed updated by people who're working on other crates.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

I think its good now, thanks!

How about putting this info somewhere else like ./CONTRIBUTING.md instead of in this crate? As it is supposed updated by people who're working on other crates.

Then lets put it there - i dont know a better location for now.

Copy link

@nkpar nkpar left a comment

Choose a reason for hiding this comment

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

Looks good to go! Nice work! ✅

@nkpar nkpar self-requested a review February 25, 2025 19:16
@huntbounty
Copy link
Contributor Author

huntbounty commented Feb 26, 2025

Excited to get this merged! What's left to be done here? Do I need add a prdoc for this? (It doesn't seem like it needs one as it doesn't change any code, but I'm not sure 😝)

@iulianbarbu iulianbarbu added the R0-silent Changes should not be mentioned in any release notes label Feb 26, 2025
@iulianbarbu
Copy link
Contributor

What's left to be done here?

My only comment is that the docs/sdk/src/reference_docs/umbrella_crate.rs is not saying much when compared to the README. We should include_str the README there, but I'll not insist.

@huntbounty
Copy link
Contributor Author

huntbounty commented Feb 26, 2025

My only comment is that the docs/sdk/src/reference_docs/umbrella_crate.rs is not saying much when compared to the README. We should include_str the README there, but I'll not insist.

I could do it in a follow-up, as this isn't done yet regarding the ultimate goal of syncing README.md and umbrella/src/lib.rs.

To address the failing umbrella check, maybe we change the script to only remove Cargo.toml and src instead of wiping the whole dir?

Copy link
Contributor

Review required! Latest push from author must always be reviewed

@ggwpez
Copy link
Member

ggwpez commented Feb 27, 2025

/tip small

Copy link

@huntbounty Contributor did not properly post their account address.

Make sure the pull request description (or user bio) has: "{network} address: {address}".

@ggwpez ggwpez enabled auto-merge February 27, 2025 12:55
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez added this pull request to the merge queue Feb 27, 2025
Merged via the queue into paritytech:master with commit d734e79 Feb 27, 2025
244 of 251 checks passed
@huntbounty
Copy link
Contributor Author

Yay! Thank y'all for all the patience and guidance! 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polkadot umbrella crate needs a README
5 participants