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

misc. cargo doc improvements. #153

Merged
merged 9 commits into from
Sep 11, 2023
Merged

misc. cargo doc improvements. #153

merged 9 commits into from
Sep 11, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Sep 9, 2023

Small improvements to the existing rustdocs. I recommend reviewing commit-by-commit.

  • Fixing redundant link targets in src/key_pair.rs flagged by beta rust doc.
  • Fixing a typo in `src/key_pair.rs.
  • Updating the permitted/excluded subtree name constraint docs to be consistent in their descriptions.
  • Applying consistent comment standards to the crate internal src/oid.rs comments.
  • Adding rustdoc comments to CertificateSigningRequest, removing an allow for missing docs.
  • Pulling out a separate rust doc CI job, testing with stable, beta, nightly and MSRV toolchains on one platform.
  • Adding --document-private-items to the CI rust doc job to catch crate-internal doc errs.
  • Enabling doc_auto_cfg to get automatic feature requirement display in the doc.rs docs
  • Removing redundant manually maintained feature requirement comments in favour of doc_auto_cfg's automatic support.

@cpu cpu self-assigned this Sep 9, 2023
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

Okay except comments.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
src/oid.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Sep 10, 2023

Okay except comments.

Thanks for the review. I believe I addressed all of your comments.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Nice!

src/lib.rs Outdated Show resolved Hide resolved
Matches language across the `permitted_subtrees` and `excluded_subtrees`
rustdoc comments for the `NameConstraints` struct.
Updates the crate internal rustdoc comments for the various OID
constants so that:

* each uses the same format of rustdoc comment.
* each has the name the OID uses in the cited doc.
* each has a citation link to the defining doc.
* each citation link uses the ASN.1 module definition, not the doc
  section explaining the usage of the associated feature.
Previously `cargo doc` was being invoked in the per-platform CI jobs.
This feels unnecessary since no documentation items are gated
per-platform and we can rely on the upstream `cargo doc` tool owners to
test for general doc generation platform compatibility.

In this commit the `cargo doc` invocation is moved to a dedicated job.
This also allows shifting the `RUSTDOCFLAGS` env var from a global env
var to one scoped to just the job that requires it.

We continue to test doc generation using the stable, beta, nightly and
MSRV rust versions.
This commit updates the `cargo doc` CI job to run the tool with
`--document-private-items`, testing that cargo doc comments on crate
internal items are free from error. These comments, while not part of
the public API docs, can be helpful for crate developers and should be
kept tidy where they exist.
The upstream `docs.rs` website uses the nightly release of the Rust
compiler when building project docs. That means we can opt in to the
`doc_auto_cfg` feature to have documentation items automatically mention
the required crate features where appropriate.

This has the advantages of:

* Using a nice styled UI element for the requirements.
* Keeping the feature requirements in-sync with the docs automatically.

Since this feature requires the nightly toolchain we customize the CI
environment so that it is enabled only for the nightly channel.

[0]: https://docs.rs/about/builds
Now that we've enabled `doc_auto_cfg` these comments are redundant with
the auto-generated `docs.rs` notes about required features.
@cpu cpu added this pull request to the merge queue Sep 11, 2023
Merged via the queue into rustls:main with commit 31403a4 Sep 11, 2023
13 checks passed
@cpu cpu deleted the cpu-cargo-doc-touchups branch September 11, 2023 16:26
@cpu
Copy link
Member Author

cpu commented Sep 11, 2023

@est31 Just wanted to mention that I enabled the "require merge queue" setting for the repository. It's convenient to be able to queue PRs to automatically merge when CI passes (after the required reviews are in) vs. having to babysit their progress. As usual, repository administrators can bypass this requirement.

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.

3 participants