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

rustdoc: skip #[allow(missing docs)] for docs in coverage report #76196

Merged
merged 5 commits into from
Oct 13, 2020
Merged

rustdoc: skip #[allow(missing docs)] for docs in coverage report #76196

merged 5 commits into from
Oct 13, 2020

Conversation

r-52
Copy link
Contributor

@r-52 r-52 commented Sep 1, 2020

During the document coverage reporting with:

rustdoc something.rs -Z unstable-options --show-coverage

the coverage report counts code that is marked with #[allow(missing_docs)] for the calculation, which outputs lower numbers in the coverage report even though these parts should be ignored for the calculation.

Right now I'm not sure how this can be tested (CI)? (I verified it by hand and ran the unit tests)

r? @jyn514

Reference: Fixes #76121

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2020
@jyn514 jyn514 added A-doc-coverage Area: Calculating how much of a crate has documentation T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 1, 2020
jyn514
jyn514 reviewed Sep 1, 2020
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.

Right now I'm not sure how this can be tested (CI)? (I verified it by hand and ran the unit tests)

You can test it by adding a file to src/test/rustdoc-ui/coverage/ that uses doc(hidden). Not sure how to generate the .stdout files, maybe run x.py test src/test/rustdoc-ui/coverage/ --bless?

src/librustdoc/passes/calculate_doc_coverage.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/calculate_doc_coverage.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Sep 1, 2020

See also https://rustc-dev-guide.rust-lang.org/rustdoc.html#cheat-sheet and https://rustc-dev-guide.rust-lang.org/tests/adding.html#guide-to-the-ui-tests

@r-52 r-52 force-pushed the r-coverage-allow-missing-docs branch from 34a576d to 1375fcb Compare September 1, 2020 13:12
@r-52
Copy link
Contributor Author

r-52 commented Sep 1, 2020

See also https://rustc-dev-guide.rust-lang.org/rustdoc.html#cheat-sheet and https://rustc-dev-guide.rust-lang.org/tests/adding.html#guide-to-the-ui-tests

@jyn514 thanks for the help and the suggestions! I'll take a look and ping you when I'm done 👍

@r-52 r-52 force-pushed the r-coverage-allow-missing-docs branch from 1375fcb to 3fc8ae6 Compare September 1, 2020 13:46
@r-52 r-52 force-pushed the r-coverage-allow-missing-docs branch from cd5eec7 to 81f65a8 Compare September 1, 2020 14:01
@ollie27
Copy link
Member

ollie27 commented Sep 1, 2020

#[allow(missing_docs)] also applies to inner items so I would expect it to do the same with --show-coverage. For example I would expect the following to print 100% coverage:

#![deny(missing_docs)]

//! crate docs

#[allow(missing_docs)]
pub mod foo {
    pub struct Foo;
}

@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 Sep 1, 2020
@r-52 r-52 force-pushed the r-coverage-allow-missing-docs branch from 81f65a8 to 3a49711 Compare September 3, 2020 13:53
@r-52 r-52 requested a review from jyn514 September 3, 2020 13:54
@r-52
Copy link
Contributor Author

r-52 commented Sep 3, 2020

@ollie27 thanks for the input! That was indeed a part that I missed, but it's covered now 👍

@JohnCSimon JohnCSimon 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 Sep 21, 2020
@Dylan-DPC-zz Dylan-DPC-zz 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 Sep 22, 2020
@crlf0710
Copy link
Member

crlf0710 commented Oct 8, 2020

@r-52 Ping from triage, would you mind address the review comments above? thanks!

@r-52
Copy link
Contributor Author

r-52 commented Oct 8, 2020

@crlf0710 hi, thanks the ping! Right now, I don't have the time to work on it. Feel free to pick it up

@GuillaumeGomez
Copy link
Member

I'll finish it then.

@GuillaumeGomez GuillaumeGomez force-pushed the r-coverage-allow-missing-docs branch from 3a49711 to 5969b38 Compare October 8, 2020 16:20
@GuillaumeGomez
Copy link
Member

Ok, updated!

@r-52 @GuillaumeGomez
rustdoc: skip allow missing doc in cover. report
02e6b86 
During the document coverage reporting with
```bash
rustdoc something.rs -Z unstable-options --show-coverage
```

the coverage report also includes parts of the code that are marked
with `#[allow(missing_docs)]`, which outputs lower numbers in the
coverage report even though these parts should be ignored for the
calculation.

Co-authored-by: Joshua Nelson <joshua@yottadb.com>
@GuillaumeGomez GuillaumeGomez force-pushed the r-coverage-allow-missing-docs branch from 5969b38 to 6854440 Compare October 12, 2020 12:33
@jyn514
Copy link
Member

jyn514 commented Oct 12, 2020

@bors r+

Looks great!

@bors
Copy link
Contributor

bors commented Oct 12, 2020

📌 Commit 6854440 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 13, 2020
@Dylan-DPC
Rollup merge of rust-lang#76196 - r-52:r-coverage-allow-missing-docs,…
19ca234 
… r=jyn514

rustdoc: skip #[allow(missing docs)] for docs in coverage report

During the document coverage reporting with:
```bash
rustdoc something.rs -Z unstable-options --show-coverage
```

the coverage report counts code that is marked with `#[allow(missing_docs)]` for the calculation, which outputs lower numbers in the coverage report even though these parts should be ignored for the calculation.

Right now I'm not sure how this can be tested (CI)? (I verified it by hand and ran the unit tests)

r? @jyn514

**Reference:** Fixes rust-lang#76121
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Oct 13, 2020
Closed
@bors
Copy link
Contributor

bors commented Oct 13, 2020

⌛ Testing commit 6854440 with merge 4d63435...

@bors
Copy link
Contributor

bors commented Oct 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 4d63435 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 13, 2020
@bors bors merged commit 4d63435 into rust-lang:master Oct 13, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 13, 2020
This was referenced Oct 13, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

@ollie27 ollie27 ollie27 left review comments

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
A-doc-coverage Area: Calculating how much of a crate has documentation 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.49.0
Development

Successfully merging this pull request may close these issues.

[rustdoc] When calculating docs percentage, ignore items marked allow(missing_docs)
10 participants
@r-52 @rust-highfive @jyn514 @ollie27 @crlf0710 @GuillaumeGomez @bors @JohnCSimon @Dylan-DPC-zz @rustbot