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 document-private-items flag to cargo doc #5543

Merged
merged 3 commits into from Jul 16, 2018

Conversation

Projects
None yet
5 participants
@roblabla
Contributor

roblabla commented May 16, 2018

Add a --document-private-items flag to cargo doc, that mimics the equivalent cargo rustdoc -- --document-private-items. This works by relaying the flag to the underlying rustdoc call.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive May 16, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rust-highfive commented May 16, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 28, 2018

Member

Could this be handled by using cargo rustdoc -- --document-private-itmes?

Member

matklad commented May 28, 2018

Could this be handled by using cargo rustdoc -- --document-private-itmes?

@roblabla

This comment has been minimized.

Show comment
Hide comment
@roblabla

roblabla May 28, 2018

Contributor

I tried to use it, but couldn't figure out how to make it work. Reason is, I'm using xargo and my crate only compiles for a specific target (it errors out on others). So to document it, I need the ability to specify a specific target, e.g. xargo doc --target=i386-unknown-none. Doing so, I lose the ability to document private items, hence the feature request. xargo rustdoc doesn't work because I can't specify a target, and using rustdoc itself means I need to specify a sysroot manually...

I'm pretty sure I could somehow figure out the correct sequence of commands to get rustdoc to do what I want, but it seemed easier to just add the ability to document private items in cargo doc.

Contributor

roblabla commented May 28, 2018

I tried to use it, but couldn't figure out how to make it work. Reason is, I'm using xargo and my crate only compiles for a specific target (it errors out on others). So to document it, I need the ability to specify a specific target, e.g. xargo doc --target=i386-unknown-none. Doing so, I lose the ability to document private items, hence the feature request. xargo rustdoc doesn't work because I can't specify a target, and using rustdoc itself means I need to specify a sysroot manually...

I'm pretty sure I could somehow figure out the correct sequence of commands to get rustdoc to do what I want, but it seemed easier to just add the ability to document private items in cargo doc.

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 28, 2018

Member

Hm, looks like this is a bug in cargo rustdoc, it should support --target. Filed #5587 for that.

Member

matklad commented May 28, 2018

Hm, looks like this is a bug in cargo rustdoc, it should support --target. Filed #5587 for that.

bors added a commit that referenced this pull request May 28, 2018

Auto merge of #5587 - matklad:rustdoc-target, r=alexcrichton
Support `--target` argument in `cargo rustdoc`

We don't support `--target` in `cargo rustdoc`. Seems like an omission to me? We support it in `cargo rustc`. Discovered in #5543 (comment).
@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 28, 2018

Member

@roblabla now cargo rustdoc supports the —target flag. Will it work for your case?

Member

matklad commented May 28, 2018

@roblabla now cargo rustdoc supports the —target flag. Will it work for your case?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 5, 2018

Member

ping @roblabla, any updates on this PR? It looks like it may be ready to go with a test?

Member

alexcrichton commented Jul 5, 2018

ping @roblabla, any updates on this PR? It looks like it may be ready to go with a test?

@roblabla

This comment has been minimized.

Show comment
Hide comment
@roblabla

roblabla Jul 5, 2018

Contributor

Yeah, this PR should work (I've been using it in a personal project for a while now). I'll add a test case. I suppose it would go in tests/testsuite/doc.rs ?

Contributor

roblabla commented Jul 5, 2018

Yeah, this PR should work (I've been using it in a personal project for a while now). I'll add a test case. I suppose it would go in tests/testsuite/doc.rs ?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 5, 2018

Member

Indeed!

Member

alexcrichton commented Jul 5, 2018

Indeed!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 13, 2018

Member

@roblabla any luck adding the test case?

Member

alexcrichton commented Jul 13, 2018

@roblabla any luck adding the test case?

@roblabla

This comment has been minimized.

Show comment
Hide comment
@roblabla

roblabla Jul 15, 2018

Contributor

Alright, sorry for the long wait, I finally got around to adding a proper test case for this.

Contributor

roblabla commented Jul 15, 2018

Alright, sorry for the long wait, I finally got around to adding a proper test case for this.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 16, 2018

Member

@bors: r+

Thanks!

Member

alexcrichton commented Jul 16, 2018

@bors: r+

Thanks!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 16, 2018

Contributor

📌 Commit 7757753 has been approved by alexcrichton

Contributor

bors commented Jul 16, 2018

📌 Commit 7757753 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 16, 2018

Contributor

⌛️ Testing commit 7757753 with merge 06721dd...

Contributor

bors commented Jul 16, 2018

⌛️ Testing commit 7757753 with merge 06721dd...

bors added a commit that referenced this pull request Jul 16, 2018

Auto merge of #5543 - roblabla:doc-private-items, r=alexcrichton
Add document-private-items flag to cargo doc

Add a `--document-private-items` flag to `cargo doc`, that mimics the equivalent `cargo rustdoc -- --document-private-items`. This works by relaying the flag to the underlying rustdoc call.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jul 16, 2018

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 06721dd to master...

Contributor

bors commented Jul 16, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 06721dd to master...

@bors bors merged commit 7757753 into rust-lang:master Jul 16, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment