Skip to content

Conversation

phlip9
Copy link
Contributor

@phlip9 phlip9 commented Jun 12, 2025

While upgrading to v0.12.0 our cargo doc builds broke:

$ cargo doc --no-default-features --features=async,tokio

# ...

error: unresolved link to `minreq`
  --> /Users/phlip9/dev/esplora-client/src/lib.rs:54:42
   |
54 | //! * `blocking-https-bundled` enables [`minreq`], the blocking client with proxy and TLS (SSL)
   |                                          ^^^^^^ no item named `minreq` in scope
   |
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: public documentation for `max_retries` links to private item `RETRYABLE_ERROR_CODES`
   --> /Users/phlip9/dev/esplora-client/src/lib.rs:171:21
    |
171 |     /// is one of [`RETRYABLE_ERROR_CODES`].
    |                     ^^^^^^^^^^^^^^^^^^^^^ this item is private

# ... etc

To keep the nice doc links to minreq and reqwest, I just pointed them to https://docs.rs/minreq and https://docs.rs/reqwest respectively. It won't resolve to the exact version, but it's better than nothing IMO.

@phlip9
Copy link
Contributor Author

phlip9 commented Jun 12, 2025

Looks like MSRV CI isn't pinning lock_api to the right version?

@coveralls
Copy link

coveralls commented Jun 12, 2025

Pull Request Test Coverage Report for Build 15917138414

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 87.594%

Totals Coverage Status
Change from base Build 15916798408: 0.0%
Covered Lines: 1052
Relevant Lines: 1201

💛 - Coveralls

@ValuedMammal
Copy link
Contributor

I'm updating the pinned deps in #130 if you want to try to build this PR on top of that.

@phlip9
Copy link
Contributor Author

phlip9 commented Jun 17, 2025

rebased on #130

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 03de52e

@notmandatory notmandatory added the documentation Improvements or additions to documentation label Jun 27, 2025
notmandatory
notmandatory previously approved these changes Jun 27, 2025
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the docs here. I have a couple suggestions to keep the original intent when the docs are built for docs.rs.

@notmandatory notmandatory self-requested a review June 27, 2025 01:02
@notmandatory notmandatory dismissed their stale review June 27, 2025 01:02

I didn't mean to approve it here, just meant to add my comments.

@notmandatory
Copy link
Member

You'll also need to rebase on master now that #130 is merged. Thanks!

@phlip9
Copy link
Contributor Author

phlip9 commented Jun 27, 2025

Hmm, the nightly rustfmt wants to remove this newline (though my local toolchain doesn't yet):

 //! * `async-https-rustls-manual-roots` enables [`reqwest`], the async client with support for
 //!   proxying and TLS (SSL) using the `rustls` TLS backend without using its the default root
 //!   certificates.
-//!
 #![cfg_attr(not(feature = "minreq"), doc = "[`minreq`]: https://docs.rs/minreq")]
 #![cfg_attr(not(feature = "reqwest"), doc = "[`reqwest`]: https://docs.rs/reqwest")]
 #![allow(clippy::result_large_err)]

But then cargo doc fails without the newline : /

$ cargo doc --no-default-features --features=async,tokio
 Documenting esplora-client v0.12.0 (/Users/phlip9/dev/esplora-client)
warning: unresolved link to `minreq`
  --> src/lib.rs:1:1
   |
1  | / //! An extensible blocking/async Esplora client
2  | | //!
3  | | //! This library provides an extensible blocking and
4  | | //! async Esplora client to query Esplora's backend.
...  |
65 | | //!   certificates.
66 | | #![cfg_attr(not(feature = "minreq"), doc = "[`minreq`]: https://docs.rs/minreq\n")]
   | |_________________________________________________________________________________^
   |
   = note: the link appears in this line:

           client using [`minreq`] and an async client using [`reqwest`].
                         ^^^^^^^^
   = note: no item named `minreq` in scope
   = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
   = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

@phlip9
Copy link
Contributor Author

phlip9 commented Jun 27, 2025

Nice, got it to work with this dummy link lol

```diff
 //! * `async-https-rustls-manual-roots` enables [`reqwest`], the async client with support for
 //!   proxying and TLS (SSL) using the `rustls` TLS backend without using its the default root
 //!   certificates.
 //!
+//! [`dont remove this line or cargo doc will break`]: https://example.com
 #![cfg_attr(not(feature = "minreq"), doc = "[`minreq`]: https://docs.rs/minreq")]
 #![cfg_attr(not(feature = "reqwest"), doc = "[`reqwest`]: https://docs.rs/reqwest")]
 #![allow(clippy::result_large_err)]

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 6eece87

Thanks for the quick turnaround and figuring out that dummy link fix.

@notmandatory notmandatory merged commit 900d508 into bitcoindevkit:master Jun 28, 2025
25 checks passed
@phlip9 phlip9 deleted the phlip9/fix-doc branch July 1, 2025 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants