Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

doc-only module to group all storage items of a pallet #12398

Closed
Tracked by #264
kianenigma opened this issue Oct 2, 2022 · 15 comments · Fixed by #13341
Closed
Tracked by #264

doc-only module to group all storage items of a pallet #12398

kianenigma opened this issue Oct 2, 2022 · 15 comments · Fixed by #13341
Labels
J0-enhancement An additional feature request.

Comments

@kianenigma
Copy link
Contributor

A simple one: create a

#[cfg(doc)]
pub mod storage_types {
   pub use crate::pallet::every_public_storage_item;
}

for nicer discoverability in rustdocs.

Essentially, similar to this page, but better:

https://polkadot.js.org/docs/substrate/storage/#staking

@kianenigma kianenigma added the J0-enhancement An additional feature request. label Oct 2, 2022
@kianenigma kianenigma moved this to Backlog in Runtime / FRAME Oct 2, 2022
@kianenigma
Copy link
Contributor Author

cc @sam0x17

@sam0x17
Copy link
Contributor

sam0x17 commented Oct 2, 2022

This would be a good add-on or follow-up to #12334

@bkchr
Copy link
Member

bkchr commented Oct 2, 2022

We should not only list all the public storage types. We should probably support having all of them shown in the docs, not just the public ones.

@kianenigma
Copy link
Contributor Author

We should probably support having all of them shown in the docs, not just the public ones.

Yeah, let's do that. Might be a little confusing though because some of them might not be public in the Rust code.

This pub mod storage_types {} should have a clear documentation saying that what's named here might not be publicly available in the API and is only listed as reference.

@kianenigma
Copy link
Contributor Author

This would be a good add-on or follow-up to #12334

I think they are rather unrelated in terms of what needs to be done for it, but yeah perhaps a follow-up for you once you are finished there, feel free to assign to yourself :)

@kianenigma
Copy link
Contributor Author

@olliecorbisiero this is the fix for what we spoke of today.

@kianenigma
Copy link
Contributor Author

This is currently blocked by the fact that some storage types can be made pub(crate).

Option 1: All storage items must be public and visible in the rust-docs.
Option 2: All storage items are artificially #[cfg(doc)] pub #[cfg(not(doc))] $vis. A bit confusing eh?
Option 3: non-public storage items will not go into rust-docs.

I would go with either option 1 or 3 for now.

@bkchr
Copy link
Member

bkchr commented Feb 7, 2023

Option 4: cargo doc --document-private-items

Option 3: non-public storage items will not go into rust-docs.

Sounds reasonable, as you only see the storage items that you can access.

Option 1: All storage items must be public and visible in the rust-docs.

We could add some warning when you use something that isn't pub :D A lot of people always complain that storage items aren't public, which doesn't make that much sense as when you know they key you can just modify all of them.

@kianenigma
Copy link
Contributor Author

@ggwpez @KiChjang @shawntabrizi if you are more or less on the same page, let's consider option 1.

@KiChjang
Copy link
Contributor

KiChjang commented Feb 8, 2023

Well, in that case, why don't we just remove the ability to set visibility on storages? Sounds like we can just modify the macro to do so.

@bkchr
Copy link
Member

bkchr commented Feb 8, 2023

We could, but a warning is probably better from the usability.

@sam0x17
Copy link
Contributor

sam0x17 commented Feb 8, 2023

Bear in mind custom warnings aren't stabilized in rust yet (unless this happened very recently without me knowing) so this is often harder than it sounds

@bkchr
Copy link
Member

bkchr commented Feb 8, 2023

Yeah I know, but yeah, we could also make it a compile_error. However, we should ensure that we generate the correct code plus the compile error. Otherwise people would get tons of unrelated compile errors.

@kianenigma
Copy link
Contributor Author

@sam0x17 if you want to pickup #13341 and related tasks, please feel free to do so.

@sam0x17
Copy link
Contributor

sam0x17 commented Feb 14, 2023

@kianenigma Yeah I'll pick that up. It's well timed because I'm wrapping up #13224 today

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J0-enhancement An additional feature request.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants