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

Create rustdoc_internals feature gate #90420

Merged
merged 4 commits into from
Nov 25, 2021
Merged

Create rustdoc_internals feature gate #90420

merged 4 commits into from
Nov 25, 2021

Conversation

GuillaumeGomez
Copy link
Member

As suggested by @camelid here, since doc_keyword and doc_primitive aren't meant to be stabilized, we could put them behind a same feature flag.

This is pretty much what it would look like (needs to update the tests too).

The tracking issue is #90418.

What do you think @rust-lang/rustdoc ?

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. requires-nightly This issue requires a nightly compiler in some way. labels Oct 30, 2021
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member

camelid commented Oct 30, 2021

r? @camelid

camelid
camelid reviewed Oct 30, 2021
View reviewed changes
compiler/rustc_feature/src/active.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_passes/src/feature_gate.rs Outdated Show resolved Hide resolved
@camelid camelid 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 Oct 30, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 30, 2021

This seems ... kind of unnecessary to me? We can document on the tracking issues that these aren't meant to be stabilized, I don't think it also needs to be in the source code of everything using them. And as you've noticed the logic for doc_primitive is pretty different from that of doc_keyword, so I don't think it's particularly useful to combine them.

@camelid
Copy link
Member

camelid commented Oct 30, 2021 •
edited
Loading

This seems ... kind of unnecessary to me? We can document on the tracking issues that these aren't meant to be stabilized, I don't think it also needs to be in the source code of everything using them.

The standard library often uses the naming convention of X_internals to indicate that something is perma-unstable. Whereas, #![feature(doc_keyword)] doesn't give any indication that it's only meant for internal use. People don't necessarily look at tracking issues, so IMO it's best to indicate in the feature name that something is internal-only. Another example is #[rustc_layout(debug)] which is under the feature-gate rustc_attrs, not something like debug_layout.

If merging the feature gates would be too complicated, I'm okay to leave them separate, but I would prefer to merge them and make it clear in the name that they are internal-only.

I don't think it also needs to be in the source code of everything using them.

They're only used in the standard library and rustdoc IIUC, and this would reduce the number of feature gates necessary?

@jyn514
Copy link
Member

jyn514 commented Oct 30, 2021

Hmm, ok, seems reasonable.

@jyn514 jyn514 mentioned this pull request Oct 30, 2021
Merged
@bors

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez marked this pull request as ready for review November 8, 2021 19:00
@GuillaumeGomez
Copy link
Member Author

Updated the message and set the PR as ready to review.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Done!

camelid
camelid approved these changes Nov 21, 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.

Left a few comments. r=me after fixing those.

compiler/rustc_feature/src/active.rs Outdated
Comment on lines 164 to 165
/// Allows using internal rustdoc features like `doc(primitive)` or `doc(keyword)`.
(active, rustdoc_internals, "1.58.0", Some(90418), None),
Copy link
Member

Choose a reason for hiding this comment

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

This is in the wrong section; it should be next to doc_notable_trait.

Copy link
Member

Choose a reason for hiding this comment

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

Also, can you create an actual tracking issue? We shouldn't just link to this PR.

compiler/rustc_feature/src/removed.rs Outdated
/// Allows using `#[doc(keyword = "...")]`.
(removed, doc_keyword, "1.28.0", Some(51315), None,
Some("merged into `#![feature(rustdoc_internals)]`")),
/// Allows using doc(primitive) without a future-incompat warning
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
/// Allows using doc(primitive) without a future-incompat warning
/// Allows using `doc(primitive)` without a future-incompat warning

@camelid camelid 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 21, 2021
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Nov 21, 2021 •
edited
Loading

Also, can you create an actual tracking issue? We shouldn't just link to this PR.

It does link to the tracking issue. Unless I missed something?

Otherwise, I updated following your comments.

@camelid
Copy link
Member

camelid commented Nov 21, 2021

Sorry, maybe GitHub glitched and showed me the wrong page. Tracking issue looks good to me.

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 21, 2021
camelid
camelid approved these changes Nov 24, 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.

r=me with these two comments fixed

compiler/rustc_feature/src/active.rs Outdated Show resolved Hide resolved
compiler/rustc_feature/src/removed.rs Outdated Show resolved Hide resolved
@camelid camelid 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 24, 2021
@GuillaumeGomez
Copy link
Member Author

@camelid Your comments made perfect sense (and I have no clue how I messed up the indent...). Updated!

@camelid
Copy link
Member

camelid commented Nov 24, 2021

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 24, 2021

📌 Commit 9447d1f has been approved by camelid

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 24, 2021
@GuillaumeGomez
Rollup merge of rust-lang#90420 - GuillaumeGomez:rustdoc-internals-fe…
f52f0e5 
…ature, r=camelid

Create rustdoc_internals feature gate

As suggested by `@camelid` [here](rust-lang#90398 (comment)), since `doc_keyword` and `doc_primitive` aren't meant to be stabilized, we could put them behind a same feature flag.

This is pretty much what it would look like (needs to update the tests too).

The tracking issue is rust-lang#90418.

What do you think `@rust-lang/rustdoc` ?
This was referenced Nov 24, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 24, 2021
@bors
Auto merge of rust-lang#91203 - GuillaumeGomez:rollup-kwtqvb1, r=Guil…
d2c24aa 
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#89542 (Partially stabilize `duration_consts_2`)
 - rust-lang#90044 (Restrict aarch64 outline atomics to glibc for now.)
 - rust-lang#90420 (Create rustdoc_internals feature gate)
 - rust-lang#91075 (Reduce prominence of item-infos)
 - rust-lang#91151 (Fix test in std::process on android)
 - rust-lang#91179 (Fix more <a> color)
 - rust-lang#91199 (rustdoc: Add test for mixing doc comments and attrs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a6a1d7c into rust-lang:master Nov 25, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 25, 2021
@GuillaumeGomez GuillaumeGomez deleted the rustdoc-internals-feature branch November 25, 2021 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@camelid camelid camelid approved these changes

Assignees

@camelid camelid

Labels
requires-nightly This issue requires a nightly compiler in some way. 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.58.0
Development

Successfully merging this pull request may close these issues.
8 participants
@GuillaumeGomez @rust-highfive @rust-log-analyzer @camelid @jyn514 @bors @michaelwoerister @rustbot