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 Rust documentation check to GitHub Actions #937

Merged
merged 6 commits into from Jul 11, 2022
Merged

Conversation

cycraig
Copy link
Contributor

@cycraig cycraig commented Jul 8, 2022

Description of change

Build Rust documentation in CI to catch broken links early. The new check is relatively fast at ~5 minutes, so should not slow down the overall CI, especially when run in parallel with much larger jobs.

Type of change

  • Chore/Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

To test locally, change one of the documentation links in a Rust file to be incorrect.
Then run:

RUSTDOCFLAGS="-D warnings" cargo doc --all-features --no-deps --workspace

It should print an error similar to the one below and exit with code 1:

error: unresolved link to `crate::validator::PresentationValidator`
 --> identity_iota_client\src\lib.rs:6:1
  |
6 | #![doc = include_str!("./../README.md")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
  = note: the link appears in this line:

          - [`PresentationValidator`](crate::validator::PresentationValidator)
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = note: no item named `validator` in module `identity_iota_client`

error: could not document `identity_iota_client`

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@cycraig cycraig requested a review from eike-hass July 8, 2022 17:36
@cycraig cycraig added the No changelog Excludes PR from becoming part of any changelog label Jul 8, 2022
Comment on lines 39 to 43
env:
RUSTDOCFLAGS: "-D warnings"
with:
command: doc
args: --all-features --no-deps --workspace
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we build it the way docs.rs builds the documentation, so we don't run into the 0.6 doc problems again?
So specifically using nightly and --cfg docsrs. So:

RUSTDOCFLAGS='-D warnings --cfg docsrs' cargo +nightly doc --all-features --no-deps --workspace

Copy link
Contributor Author

@cycraig cycraig Jul 11, 2022

Choose a reason for hiding this comment

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

Good point, will do done.

Is the docsrs config really necessary in our code though?
For example, the double cfg features in identty_iota?

#[cfg(feature = "account")]
#[cfg_attr(docsrs, doc(cfg(feature = "account")))]
pub mod account_storage {

Edit: should we specify the features in Cargo.toml instead or something? Not sure what the best practice is.

E.g.

[package.metadata.docs.rs]
features = [ "account", "..." ]

Edit2: we currently have all-features though, so probably unnecessary.

[package.metadata.docs.rs]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]

Copy link
Contributor

Choose a reason for hiding this comment

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

The double-cfg is necessary I think, since #[doc(cfg(...))] is unstable. It's how crypto.rs does it, too. I mainly copied their approach: https://github.com/iotaledger/crypto.rs/blob/94be7913f11018467edb64c3d126ab682c29edcf/src/signatures/mod.rs#L5.

Yes, I think all-features = true in the toml metadata should be fine.

@cycraig cycraig merged commit 6657c1e into dev Jul 11, 2022
@cycraig cycraig deleted the chore/rustdocs-ci branch July 11, 2022 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No changelog Excludes PR from becoming part of any changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants