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

Accessing Pallet Documentation #47

Closed
harrysolovay opened this issue Dec 11, 2022 · 15 comments · Fixed by paritytech/substrate#13452
Closed

Accessing Pallet Documentation #47

harrysolovay opened this issue Dec 11, 2022 · 15 comments · Fixed by paritytech/substrate#13452

Comments

@harrysolovay
Copy link

Pallet metadata does not contain associated documentation while the metadata of other types does. I'm guessing this is for the sake of reducing the size of the metadata... nevertheless: there does not seem to be a standard way for a developer to retrieve pallet documentation.

@bkchr
Copy link
Member

bkchr commented Dec 11, 2022

You mean the documentation of the crate itself?

@harrysolovay
Copy link
Author

Correct

@bkchr
Copy link
Member

bkchr commented Dec 11, 2022

We can not pull this into the metadata. The macros don't have access to this.

@tjjfvi
Copy link

tjjfvi commented Jan 3, 2023

The macros don't have access to this at the moment, but perhaps they could be given access? E.g.

#[frame_support::pallet]
#[doc = include_str!("../README.md")]
pub mod pallet {
  ...
}

If I'm not mistaken, this would both attach docs to the pallet module from rust's perspective as well as give the frame_support::pallet macro access to the #[doc = include_str!("../README.md")] directive, from which it could extract the include_str!("../README.md") to include in the generated code.

@bkchr
Copy link
Member

bkchr commented Jan 3, 2023

These docs would then be on top of the module and these docs could already be read right now by the macro.

@harrysolovay
Copy link
Author

these docs could already be read right now by the macro

Does this mean that we have a path forward?

@bkchr
Copy link
Member

bkchr commented Jan 4, 2023

No, because the docs are at the crate level or at the file level. These docs are never above the pallet module. We also just add this pallet module for shortcommings in rust proc macros.

In general people would like more to have the other direction, aka less docs in the metadata and not more.

@tjjfvi
Copy link

tjjfvi commented Jan 4, 2023

In general people would like more to have the other direction, aka less docs in the metadata and not more.

I suspect that people don't want "less docs in the metadata" so much as they want "doc-less metadata". And this makes perfect sense – if you don't have a use for the docs, they're needless bloat. But if you are using the docs, you want to have as much of the docs as possible, and adding pallet-level docs will not add much bloat compared to the size of the type docs.

If people would like to have doc-less metadata (which I think is definitely desirable), it should be an option – i.e. chains should expose metadata with and without docs. Those who do not want docs can request the metadata sans docs, but those who need the docs can request the full metadata.

Having docs in the metadata is very important for external tools which consume the metadata to display to developers, be it a documentation site for chain apis, in language-level doc comments in codegen, etc.

No, because the docs are at the crate level or at the file level. These docs are never above the pallet module.

Yes, the docs are at the crate level or at the file level right now. We can put these docs above the pallet module easily with

#[doc = include_str!("../README.md")]

We also just add this pallet module for shortcommings in rust proc macros.

Then it shouldn't be a problem to add a doc comment to this pallet module to address another shortcoming in rust proc macros – that they can't access the crate's docs by default.

@bkchr
Copy link
Member

bkchr commented Jan 30, 2023

After some discussion with @harrysolovay I'm fine to go forward with this. At least to find out how a possible solution could look like and if we may find out certain issues along the way.

@lexnv
Copy link
Contributor

lexnv commented Feb 22, 2023

Substrate exposes code documentation at the crate level and/or at the file level (either by README.md or //! at file).

Considering the following pallets:

  • alliance pallet contains an identical README.md and file documentation (that developers would need to keep in sync).
  • aura pallet contains an extra paragraph that is not placed in the file documentation (the references section).

We would need a standardized way to handle pallet code documentation and pallet metadata documentation.
For example, developers may want to include a few extra paragraphs to describe some internal detail that the end user shouldn't care about; and as such, that should not be included in the pallet metadata documentation.

Pallet Metadata Documentation

I'd like to propose that pallet metadata documentation is collected only from the pallet's module.
This makes it explicit which documentation is passed to the runtime metadata; and gives the pallet macro access to it.
The following formats would be supported:

#[frame_support::pallet]

#[doc = include_str!("../README.md")]
/// Maybe we need to describe another detail here.
#[doc = "Similar format"]
pub mod pallet {
  ...
}

Pallet Code Documentation

Developers have the choice to place "internal" code documentation on README.md (that may also be exposed in the pallet metadata documentation if they'd like), as well as to place it on the lib.rs directly (knowing that it will not be included in pallet metadata documentation).

Let me know what you think of it!

// CC @kianenigma @bkchr

@bkchr
Copy link
Member

bkchr commented Feb 22, 2023

Developers have the choice to place "internal" code documentation on README.md (that may also be exposed in the pallet metadata documentation if they'd like), as well as to place it on the lib.rs directly (knowing that it will not be included in pallet metadata documentation).

IMO there should not exist 1000 ways to document your pallet. There should be one that then works for docs.rs and the metadata. There is also this issue in Substrate: paritytech/substrate#13299 which is about streamlining the pallet docs and to remove stuff like what kind of Calls the pallet provides. The pallet docs should be an high level description of the pallet. As we will probably not achieve to have just one include of the README.md we probably should settle with having on #![doc = include_str!("../README.md")] at the top of lib.rs and then some special pallet_docs("../README.md") attribute for the frame macros (otherwise the docs would be duplicated in the crate).

@bkchr
Copy link
Member

bkchr commented Feb 23, 2023

BTW, today I a crazy idea: https://github.com/livioribeiro/cargo-readme

This is some tool to extract the crate docs as Readme, basically what we are trying to do here. But maybe too crazy.

@lexnv
Copy link
Contributor

lexnv commented Feb 24, 2023

Thats pretty cool! It seems to extract the documentation from the src/lib.rs in the following formats: //! and /*!.

We could probably achieve the same result with a few lines of code:

    let file = File::open("src/main.rs").expect("failed to read source");
    let reader = BufReader::new(file);
    let lines: Vec<_> = reader
        .lines()
        .filter_map(|line| {
            line.ok()
                .and_then(|line| line.strip_prefix("//!").map(|l| l.trim().to_string()))
        })
        .collect();

Coming back to this:

The first approach seems simpler ofhand, would love you hear your thoughts @bkchr 🙏

@bkchr
Copy link
Member

bkchr commented Feb 27, 2023

One thing that is currently missing in your thoughts is that we also support multiple pallets per crate. This is for example the case in Polkadot. So, we can not just put the lib.rs docs into a README.md. The problem being that we currently can not find out which file the macro is compiling.

So for now we probably still need to stick to the current approach of declaring the README manually.

@lexnv
Copy link
Contributor

lexnv commented Feb 27, 2023

Cool! Thanks for the info!

That being said, the initial ideas are implemented at: paritytech/substrate#13452.
After that's reviewed/merged, I'll raise an issue in substrate to ensure we don't forget about this: rust-lang/rust#54725.
Building on the above PR, we could accept pallet_doc without any attributes to capture the file documentation from the current invocation path.

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 a pull request may close this issue.

4 participants