-
Notifications
You must be signed in to change notification settings - Fork 834
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
base: master
Are you sure you want to change the base?
Conversation
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? |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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:
- highlight what the difference between the two is.
polkadot-sdk
should referencepolkadot-sdk-frame
and visa versa, avoiding confusion about one another - The example you have here should use
use polkadot_sdk::polkadot_sdk_frame as frame
and follow its tracks.
There was a problem hiding this 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.
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 |
No, the thing I was thinking is to merge this README and the |
Now that the generation section is ported over, the README covers almost everything in Running |
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. |
For future reference, @huntbounty, it’s a good practice to close review comments once you’ve addressed them. LGTM otherwise! |
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? |
umbrella/README.md
Outdated
|
||
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 |
There was a problem hiding this comment.
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.
umbrella/README.md
Outdated
|
||
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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! ✅
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 😝) |
My only comment is that the |
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? |
Review required! Latest push from author must always be reviewed |
Resolves #7536