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

Followup test for checking propagated documentation #514

Merged
merged 44 commits into from Jun 17, 2022
Merged

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Apr 22, 2022

This PR adds a test that compares the raw documentation, from a
RuntimeMetadataV14 object, with the documentation propagated
to the RuntimeApi.

The documentation is extracted from each Type of the RuntimeMetadataV14, which
is obtained via the test-runtime crate. A new RuntimeApi is generated locally
to not rely on hardcoded paths to the codegen/polkadot.rs interface, nor make
any assumptions about the underlying node, may it be Substrate or Polkadot.

The test provided valuable insights into the missing documentation that should have
been propagated to the RuntimeApi.

Changes to the API

#[doc = "\n\t\t\tThe [event](https://docs.substrate.io/v3/runtime/events-and-errors) emitted\n\t\t\tby this pallet.\n\t\t\t"]
^^^^^ was missing

pub enum Event {


#[doc = "\n\t\t\tCustom [dispatch errors](https://docs.substrate.io/v3/runtime/events-and-errors)\n\t\t\tof this pallet.\n\t\t\t"]
^^^^^ was missing

pub enum Error {
   #[codec(index = 0)]
   #[doc = "Inclusion inherent called more than once per block."]


#[doc = "Contains one variant per dispatchable that can be called by an extrinsic."]
^^^^^ was missing

pub enum Call {
   #[codec(index = 0)]
   #[doc = "Service a single overweight upward message."]

lexnv added 30 commits April 22, 2022 12:43
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
This reverts commit 5460bbafc6262ac4f53e11d4ef11e44e1ce8296f.
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
…ests

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team May 17, 2022 10:29
@@ -2445,6 +2450,7 @@ pub mod api {
}
}
}
#[doc = "\n\t\t\tThe [event](https://docs.substrate.io/v3/runtime/events-and-errors) emitted\n\t\t\tby this pallet.\n\t\t\t"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder whether we should look at the lines here and strip any common spacing (eg in this case, every line begins with 3 tabs, so perhaps we should remove 3 tabs from each line?

Also, separately, perhaps it makes sense to remove any leading and trailing newlines?

I'm not too bothered if this doesn't happen though, since the documentation isn't visible in the "common" macro usage, and only really becomes useful if a user generates the code manually.

Comment on lines 93 to 99
// Generated documentation will escape special characters.
// Replace escaped characters with unescaped variants for
// exact matching on the raw metadata documentation.
doc.as_str()
.replace("\\n", "\n")
.replace("\\t", "\t")
.replace("\\\"", "\"")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that all generated docs end up on one line? Should we split it over multiple lines. I wonder how the docs actually look when opened in cargo doc --open for instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The \n and \t cases originate from the substrate generation of the documentation

event_item.attrs.push(syn::parse_quote!(
			#[doc = r"
			The [event](https://docs.substrate.io/v3/runtime/events-and-errors) emitted
			by this pallet.
			"]
		));

leading to a single line

#[doc = "\n\t\t\tThe [event](https://docs.substrate.io/v3/runtime/events-and-errors) emitted\n\t\t\tby this pallet.\n\t\t\t"]

However, those lines are rendered correctly by the cargo doc --open.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect, thank you :) we can tidy up the extraneous space characters in a future PR then!

@lexnv lexnv self-assigned this May 26, 2022
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
// Documentation lines have the following format:
// # [ doc = "Upward message is invalid XCM."]
// Given the API is generated on a single line, the regex matching
// must be lazy hence the `?` in the matched group `(.*?)`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess if any doc string contains "] we'd hit an issue here?

perhaps it could also be:

let re = Regex::new(r#"\# \[doc = "[^\n]*"\]"#).unwrap();

To just explicitly forbid actual newlines from appearing and force the match to be on one line, for a bit of added reliability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a try printing all the documentation pulled from the generated file
Seems to be also handling the cases for extra ]:

			The [event](https://docs.substrate.io/v3/runtime/events-and-errors) emitted
			by this pallet.
# <weight>
- `O(1)`.

Although not entirely accurate: https://regex101.com/r/TtWQqO/1 . Seems to handle multiple ] on the same documentation line.
The ^\n didn't seem to match tho 🤔

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM, though note that in #567 I move integration tests into a sub-folder, so if that merges first this will need moving (if it doesn't autoamtically do so :))

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
fn interface_docs() -> Vec<String> {
// Generate the runtime interface from the node's metadata.
// Note: the API is generated on a single line.
let runtime_api = generate_runtime_interface();
Copy link
Member

Choose a reason for hiding this comment

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

Super cool 👍

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

lgtm

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
@lexnv lexnv merged commit 0d67b80 into master Jun 17, 2022
@lexnv lexnv deleted the 503_doc_tests branch June 17, 2022 17:25
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.

None yet

3 participants