Only document reachable APIs with cargo doc#16603
Only document reachable APIs with cargo doc#16603ojuschugh1 wants to merge 2 commits intorust-lang:masterfrom
cargo doc#16603Conversation
12fa44b to
e7d5853
Compare
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
tests/testsuite/doc_direct_deps.rs
Outdated
| pub_dep = { version = "0.0.1", public = true } | ||
| priv_dep = { version = "0.0.1", public = false } | ||
| priv_dep_with_dep = "0.0.1" | ||
| "#, |
There was a problem hiding this comment.
This is checking the mixed case for direct deps and not transitive deps. Is that intentional or did you meant for these to be indirect?
And what is it we are trying to get out of the mixed test vs other tests added?
There was a problem hiding this comment.
And what is it we are trying to get out of the mixed test vs other tests added?
Could you help me understand the role of this test?
There was a problem hiding this comment.
@ojuschugh1 there are outstanding questions on this
There was a problem hiding this comment.
Currently cargo doc generates docs for all transitive dependencies, which creates a lot of noise.
This allows library authors to control their documentation surface by
marking which dependencies are part of their public API.
The reason is that indirect private deps have no impact on the reader of the docs since they cannot be used and this can dramatically speed up documentation builds, especially with packages like windows-sys in a dependency tree
|
I have addressed your comments and feedback, kindly please review it and let me know @epage , after that i will make the commits proper. thanks |
|
@ojuschugh1 Would you mind cleaning up the commits to be atomic? (mentioned in the contributor guide) Atomic commits aren't just for Git history, they make it easier for reviewers to review changes. |
2fd32b8 to
d0ffd36
Compare
There was a problem hiding this comment.
No test updates are in this commit. Either the tests in the previous commit are failing or there is a mismatch in expectations between the tests and this change.
There was a problem hiding this comment.
This was marked as ready for review but this doesn't seem to be addressed.
There was a problem hiding this comment.
Are there any questions about this has there have been more commits pushed without addressing this.
There was a problem hiding this comment.
Apologies for the confusion! I did address the feedback:
- Simplified the boolean expression from
.require().is_ok()to.is_enabled()as you suggested - Verified all tests pass including the public-dependency tests
I should have been clearer in my commit messages about which feedback was being addressed. The code changes are now in place and working correctly. Is there anything specific you'd like me to clarify or improve further?
There was a problem hiding this comment.
Unsure if this has been said in this thread before but we find it helpful for authors, reviewers, and the community when a PR has the tests added in their own commit before the fix with them passing, reproducing the existing behavior. The commit with the fix would then update the tests to still pass and show the new behavior. The diff would show how behavior changed.
There was a problem hiding this comment.
Done in latest commit, just curious to confirm we do this before fix test + fix and updated test commit method for all the code changes PRs ?
Thanks
d0ffd36 to
a6cc3a8
Compare
a48ae5c to
31b786c
Compare
31b786c to
d327857
Compare
d327857 to
7e57124
Compare
|
Hi @epage , Gentle reminder could you kindly please reivew it, when you get a moment. Thanks |
7e57124 to
5d84a47
Compare
5d84a47 to
07b6c1b
Compare
| let should_doc_dep = if is_member || !public_deps_enabled { | ||
| true |
There was a problem hiding this comment.
Should this be is_member or is_root?
If I have
- workspace members
selectedandskipped selectedhas a private dependency onskipped- I run
cargo doc -p selected
Should we document the dependencies of skipped?
Either way, we should have a test for this.
There was a problem hiding this comment.
I think there is a hole in that example. We document dependencies by default, so the private status of skipped is irrelevant. Some layers might be needed to make the use case correct.
There was a problem hiding this comment.
Added a test doc_workspace_member_private_dep for this. Using is_member - workspace members get their deps documented regardless of how another member depends on them.
There was a problem hiding this comment.
But is that the behavior we want?
There was a problem hiding this comment.
I have renamed is_root to is_user_selected (backed by root_pkg_ids in State), which more precisely captures the intent: these are packages explicitly selected by the user (e.g. via -p), not just workspace members. The filter now only applies to deps of non-user-selected packages.
| } | ||
|
|
||
| #[cargo_test(nightly, reason = "public-dependency feature is unstable")] | ||
| fn doc_with_private_dependency() { |
There was a problem hiding this comment.
we should call out that this is a transitive private dependency
fe28062 to
6e9b1f3
Compare
| } | ||
|
|
||
| #[cargo_test(nightly, reason = "public-dependency feature is unstable")] | ||
| fn doc_workspace_member_private_dep() { |
There was a problem hiding this comment.
This test was added in the wrong commit
tests/testsuite/doc.rs
Outdated
| // foo is the selected package | ||
| // bar is a direct dep of foo | ||
| // baz is a public dep of bar | ||
| // qux is a public dep of baz | ||
|
|
There was a problem hiding this comment.
better than comments like this is to name the packages for their role in the test, e.g.
foo->selectedbar->foo-depbaz->public-bar-depqux->public-baz-dep
cargo doc
6e9b1f3 to
9ccbd9a
Compare
What does this PR try to resolve?
Fixes #2025
Adds support for documenting only direct dependencies when using the public-dependency feature. Currently cargo doc generates docs for all transitive dependencies, which creates a lot of noise. With this change, when
-Zpublic-dependencyis enabled, only direct deps and their public deps get documented.Backward compatible - without the flag everything works as before.