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

doc: remove crate:: prefix for links #1660

Merged
merged 1 commit into from
Dec 4, 2023
Merged

doc: remove crate:: prefix for links #1660

merged 1 commit into from
Dec 4, 2023

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 1, 2023

Instead, use #[cfg(doc)] to conditionally import names that we want to use in the docs. This provides a user-friendlier link name.

Also, remove rustdoc-style links from the section of src/lib.rs that gets extracted for the README, since they wind up as broken links in the README.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (74321cf) 95.90% compared to head (4915b59) 95.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1660   +/-   ##
=======================================
  Coverage   95.90%   95.90%           
=======================================
  Files          78       78           
  Lines       16083    16083           
=======================================
  Hits        15424    15424           
  Misses        659      659           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

README.md Outdated Show resolved Hide resolved
@@ -63,10 +63,10 @@
//! support WebAssembly.
//! For more information, see [the supported `ring` target platforms][ring-target-platforms].
//!
//! By providing a custom instance of the [`crate::crypto::CryptoProvider`] struct, you
//! By providing a custom instance of the crypto::CryptoProvider struct, you
Copy link
Member

Choose a reason for hiding this comment

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

So I guess the syncing of crate root docs and README is a good reason to keep them there in the README, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the question.

@ctz
Copy link
Member

ctz commented Dec 1, 2023

It's a little disappointing that [crate::Something] doesn't produce reasonable output -- maybe I'm wrong, but I'd expect the crate:: path to be dropped, as it doesn't make sense in the context of hyperlinked documentation.

@jsha
Copy link
Contributor Author

jsha commented Dec 1, 2023

Yeah, or even better, everything at the top level of the crate should be in-scope from the rustdoc perspective. That way you wouldn't have to type crate:: at all.

@ctz
Copy link
Member

ctz commented Dec 2, 2023

Maybe we could be doing:

diff --git a/rustls/src/builder.rs b/rustls/src/builder.rs
index 26de3adb..63f5a587 100644
--- a/rustls/src/builder.rs
+++ b/rustls/src/builder.rs
@@ -19,7 +19,7 @@ use std::sync::Arc;
 /// For settings besides these, see the fields of [`ServerConfig`] and [`ClientConfig`].
 ///
 /// The usual choice for protocol primitives is to call
-/// [`crate::ClientConfig::builder`]/[`ServerConfig::builder`]
+/// [`ClientConfig::builder`](crate::ClientConfig::builder)/[`ServerConfig::builder`]
 /// which will use rustls' default cryptographic provider and safe defaults for ciphersuites and
 /// supported protocol versions.
 ///

Instead of this? It's a bit more localised.

@jsha
Copy link
Contributor Author

jsha commented Dec 3, 2023

[`ClientConfig::builder`](crate::ClientConfig::builder)

This works, and we've done in in one place (which I haven't update in this PR, yet). But it gets verbose, runs into line length issues, and makes the un-rendered docs hard to read.

There's another approach:

[`ClientConfig::builder`]

...

[`ClientConfig::builder`]: crate::ClientConfig::builder

This is more concise but you wind up having to repeat it for every doc section that mentions that item. And I've found the extra typing is pretty tedious.

One additional benefit of use statements is that they can take advantage of IDE autocomplete, while comments cannot. So typing out long item names twice in comments is riskier. Also there's the potential for undetected typos (in the link text, not the link target).

@jsha jsha force-pushed the doc-no-crate branch 3 times, most recently from 7aec86d to c36323d Compare December 3, 2023 17:24
rustls/src/crypto/mod.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jsha jsha force-pushed the doc-no-crate branch 2 times, most recently from 7939906 to 9cade84 Compare December 4, 2023 17:44
@cpu
Copy link
Member

cpu commented Dec 4, 2023

@ctz Do you want to give this another pass?

Instead, use `#[cfg(doc)]` to conditionally import names that we want to
use in the docs. This provides a user-friendlier link name.
@ctz ctz enabled auto-merge December 4, 2023 17:54
@ctz ctz added this pull request to the merge queue Dec 4, 2023
Merged via the queue into rustls:main with commit 6845c01 Dec 4, 2023
23 checks passed
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

4 participants