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.

Sign up for GitHub

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

Jump to bottom

Add --out-dir flag for rustdoc #91310

Merged
merged 1 commit into from
Dec 11, 2021
Merged

Add --out-dir flag for rustdoc #91310

merged 1 commit into from
Dec 11, 2021

Conversation

Rustin170506
Copy link
Member

part of #91260

Add --out-dir flag for rustdoc and change the -o option to point to out-dir.

I'm not quite sure if it should be stable, also I'm not sure if this parameter priority is appropriate? Or should I just refuse to pass both parameters at the same time?

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 28, 2021
@jyn514
Copy link
Member

jyn514 commented Nov 28, 2021

I'm not quite sure if it should be stable,

Yes, it should be stable, that's why I opened the FCP :)

should I just refuse to pass both parameters at the same time?

What does rustc do when --out-dir is passes twice? That's what we should do here IMO.

jyn514
jyn514 suggested changes Nov 28, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Could you add a test using all three of these flags in different invocations and make sure it behaves properly? It may need to be a run-make test so you can check the generated files exist.

src/librustdoc/lib.rs Outdated Show resolved Hide resolved
jyn514
jyn514 reviewed Nov 28, 2021
View reviewed changes
src/librustdoc/config.rs Outdated Show resolved Hide resolved
src/librustdoc/config.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 28, 2021
@Rustin170506
Copy link
Member Author

@jyn514 Tests added!

Thanks for your review! 💚 💙 💜 💛 ❤️

@Rustin170506 Rustin170506 requested a review from jyn514 November 28, 2021 13:45
jyn514
jyn514 reviewed Nov 28, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Could you also add a test for when --out-dir and --output are both passed? That one can just be a rustdoc-ui test I think :)

src/test/run-make/rustdoc-with-out-dir-option/src/lib.rs Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

Could you also add a test for when --out-dir and --output are both passed? That one can just be a rustdoc-ui test I think :)

Done.

@Rustin170506 Rustin170506 requested a review from jyn514 November 28, 2021 15:50
@jyn514
Copy link
Member

jyn514 commented Nov 28, 2021

Can you squash the commits? This looks good to me after that, it just has to wait for the FCP to finish :)

camelid
camelid reviewed Nov 29, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

@hi-rustin Your "Add tests for options" commit also changes the rustdoc source code, which I found confusing when reviewing this. Could you please move those changes into your first commit and leave your second commit as just tests? Alternatively, could you squash into one commit?

src/librustdoc/config.rs Show resolved Hide resolved
src/librustdoc/lib.rs Outdated Show resolved Hide resolved
src/librustdoc/lib.rs Show resolved Hide resolved
@jyn514 jyn514 mentioned this pull request Nov 29, 2021
Closed
@Rustin170506 Rustin170506 force-pushed the rustin-patch-rustdoc branch 2 times, most recently from 5b2c973 to f5f91ee Compare November 29, 2021 02:13
@Rustin170506 Rustin170506 requested a review from camelid November 29, 2021 02:15
@Rustin170506
Copy link
Member Author

Alternatively, could you squash into one commit?

@camelid Done.
Thanks for your review! 💚 💙 💜 💛 ❤️

@camelid
Copy link
Member

camelid commented Nov 29, 2021 •
edited
Loading

Thank you @hi-rustin! One last comment: something seems to have happened to your commit message when you squashed, since it currently is this:

t # This is a combination of 2 commits.

Add --out-dir flag for rustdoc

Could you fix it so the commit is

Add --out-dir flag for rustdoc

instead? You can use git commit --amend to change the message.

@camelid
Copy link
Member

camelid commented Nov 30, 2021 •
edited
Loading

I think https://doc.rust-lang.org/nightly/rustdoc/command-line-arguments.html#-o--output-output-path needs to be updated as well. Perhaps with a <span id="-o--output-output-path"></span> alias placed above the new header too so links don't break.

@jyn514
Copy link
Member

jyn514 commented Nov 30, 2021

I think it's ok to change the fragment - it will still go to the same page, just not the right header.

@camelid
Copy link
Member

camelid commented Nov 30, 2021

I think it's ok to change the fragment - it will still go to the same page, just not the right header.

I didn't find any results for -o--output-output-path on grep.app, so yeah, it's probably fine to skip the anchor "redirect".

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 30, 2021
@Rustin170506
Add --out-dir flag for rustdoc
03be3e2 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@Rustin170506
Copy link
Member Author

I think https://doc.rust-lang.org/nightly/rustdoc/command-line-arguments.html#-o--output-output-path needs to be updated as well. Perhaps with a <span id="-o--output-output-path"></span> alias placed above the new header too so links don't break.

Updated.

@jyn514 jyn514 removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 30, 2021
@jyn514 jyn514 added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 30, 2021
@camelid
Copy link
Member

camelid commented Dec 10, 2021

FCP has finished.

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Dec 10, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 10, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 10, 2021

📌 Commit 03be3e2 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2021
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 11, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2021
@bors
Auto merge of rust-lang#91776 - matthiaskrgr:rollup-tlb4bw1, r=matthi…
b9a37ad 
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#91127 (Add `<*{const|mut} T>::{to|from}_bits`)
 - rust-lang#91310 (Add --out-dir flag for rustdoc)
 - rust-lang#91373 (Add needs-unwind to tests that depend on panicking)
 - rust-lang#91426 (Make IdFunctor::try_map_id panic-safe)
 - rust-lang#91515 (Add rsplit_array variants to slices and arrays)
 - rust-lang#91553 (socket ancillary data implementation for dragonflybsd.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d8bb4d6 into rust-lang:master Dec 11, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 Awaiting requested review from jyn514

@camelid camelid Awaiting requested review from camelid

Assignees

@jyn514 jyn514

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.59.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Rustin170506 @jyn514 @camelid @bors @rust-highfive @rustbot