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

Remove doc(include) #85457

Merged
merged 1 commit into from Jun 5, 2021
Merged

Remove doc(include) #85457

merged 1 commit into from Jun 5, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented May 19, 2021 •
edited

This nightly feature is redundant now that extended_key_value_attributes is stable (#83366). @rust-lang/rustdoc not sure if you think this needs FCP; there was already an FCP in #82539, but technically it was for deprecating, not removing the feature altogether.

This should not be merged before #83366.

cc @petrochenkov

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 May 19, 2021
jyn514
jyn514 commented May 19, 2021
View reviewed changes
src/tools/clippy/clippy_lints/src/missing_doc.rs Outdated
Comment on lines 81 to 83
let has_doc = attrs.iter().any(|a| {
a.is_doc_comment() || a.doc_str().is_some() || a.value_str().is_some() || Self::has_include(a.meta())
a.is_doc_comment() || a.doc_str().is_some() || a.value_str().is_some()
});
Copy link
Member Author

@jyn514 jyn514 May 19, 2021 •
edited

Choose a reason for hiding this comment

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

FYI @rust-lang/clippy you should probably be using rustc_lint::builtin::has_doc instead; this has a false positive and also counts things like #[path = "x"] as doc-comments (because you only check value_str().is_some(), not the name of the attribute). Also in general it seems nice to have all the logic in one place.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a nice cleanup opportunity on the Clippy side! has_doc would just have to be made public

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing, you could do both at the same time in a PR to rust-lang/rust :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened an issue: rust-lang/rust-clippy#7247

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 force-pushed the remove-doc-include branch 2 times, most recently from 7f2812e to 19b8603 Compare May 19, 2021 03:33
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

This nightly feature is redundant now that extended_key_value_attributes is stable (#83366). @rust-lang/rustdoc not sure if you think this needs FCP; there was already an FCP in #82539, but technically it was for deprecating, not removing the feature altogether.

This is a nightly feature, so I'd say that the FCP is unneeded here.

@camsteffen camsteffen mentioned this pull request May 19, 2021
Closed
@jyn514
Copy link
Member Author

jyn514 commented May 19, 2021

@GuillaumeGomez do you mind reviewing the rustdoc changes? They should be pretty simple since it's just deleted code.

Oh, I need to add a diagnostic test for suggesting doc = include_str!, let me write that up first.

@GuillaumeGomez
Copy link
Member

Oh, I need to add a diagnostic test for suggesting doc = include_str!, let me write that up first.

Not sure this is needed though. But as you prefer. Ping me once you want me 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
@jyn514 jyn514 force-pushed the remove-doc-include branch 2 times, most recently from 11c6f97 to 2a68d67 Compare May 19, 2021 19:55
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 mentioned this pull request May 20, 2021
Closed
@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
@bors

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member Author

jyn514 commented Jun 4, 2021

@GuillaumeGomez this is ready for a second round of review

@GuillaumeGomez
Copy link
Member

Thanks for this! This was a long awaited removal. :)

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 4, 2021

📌 Commit 15fec1f has been approved by GuillaumeGomez

@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 Jun 4, 2021
@bors
Copy link
Contributor

bors commented Jun 5, 2021

⌛ Testing commit 15fec1f with merge 2c10688...

@bors
Copy link
Contributor

bors commented Jun 5, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 2c10688 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 5, 2021
@bors bors merged commit 2c10688 into rust-lang:master Jun 5, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 5, 2021
@jyn514 jyn514 deleted the remove-doc-include branch June 5, 2021 12:17
@nathanregner nathanregner mentioned this pull request Jun 10, 2021
Merged
russcam pushed a commit to elastic/elasticsearch-rs that referenced this pull request Jun 18, 2021
@nathanregner
Fix README doctests on latest nightly (#175) (#176)
d96bfa2 
doc(include) was removed in rust-lang/rust#85457.
Tests should now run on on both nightly and stable.
russcam pushed a commit to elastic/elasticsearch-rs that referenced this pull request Jun 18, 2021
@nathanregner @russcam
Fix README doctests on latest nightly (#175) (#176)
4366fde 
doc(include) was removed in rust-lang/rust#85457.
Tests should now run on on both nightly and stable.

(cherry picked from commit d96bfa2)
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 22, 2021
This was referenced Jun 26, 2021
Closed
Closed
russcam pushed a commit to elastic/elasticsearch-rs that referenced this pull request Jun 29, 2021
@nathanregner @russcam
Fix README doctests on latest nightly (#175) (#176)
809ae1e 
doc(include) was removed in rust-lang/rust#85457.
Tests should now run on on both nightly and stable.
@rami3l rami3l mentioned this pull request Jul 24, 2021
Closed
2 tasks
@Badel2 Badel2 mentioned this pull request Mar 26, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 27, 2022
@Dylan-DPC
Rollup merge of rust-lang#95335 - Badel2:resolve-path, r=Dylan-DPC
979c8e8 
Move resolve_path to rustc_builtin_macros and make it private

Fixing a FIXME introduced by `@jyn514` in rust-lang#85457
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phansch phansch phansch left review comments

@camsteffen camsteffen camsteffen left review comments

@petrochenkov petrochenkov petrochenkov left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@estebank estebank

Labels
merged-by-bors This PR was explicitly merged by bors. 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.54.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@jyn514 @rust-highfive @rust-log-analyzer @bors @GuillaumeGomez @phansch @camsteffen @petrochenkov @estebank @rustbot