Skip to content

Add doc coverage information #939

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

Merged
merged 10 commits into from
Aug 12, 2020
Merged

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Aug 5, 2020

This PR adds the usage of the --show-coverage option of rustdoc. It currently looks like this:

Screenshot from 2020-08-07 00-08-51

Screenshot from 2020-08-07 00-08-42

A few things I think remains to be done (to be debated for some of them I guess?):

  • Add tests
  • Add this score in crates search results?
  • Add color code (like if it's 100% it's green and the closer to 0, the whiter?)

@GuillaumeGomez
Copy link
Member Author

Not sure to understand the issue with clippy here:

error: in an `if` condition, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
##[error]   --> src/docbuilder/rustwide_builder.rs:550:43
    |
550 |               .process_lines(&mut |line, _| {
    |  ___________________________________________^
551 | |                 if line.starts_with('{') && line.ends_with('}') {
552 | |                     json = line.to_owned();
553 | |                 }
554 | |             })
    | |_____________^
    |
    = note: `-D clippy::blocks-in-if-conditions` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_if_conditions

@Nemo157
Copy link
Member

Nemo157 commented Aug 5, 2020

Looks like a false-positive from the lint.

@GuillaumeGomez
Copy link
Member Author

And I think I forgot to add something in the migration tests, but not sure what exactly...

@GuillaumeGomez GuillaumeGomez force-pushed the doc-coverage branch 2 times, most recently from 0125a40 to 5114942 Compare August 5, 2020 14:38
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

Updated! I updated the screenshot as well.

@jyn514
Copy link
Member

jyn514 commented Aug 6, 2020

This says '50 out of 52 items documented' in the screenshot - does it have a subpage that shows which items are documented and which aren't? And if not, could you add that? Maybe /crate/:crate/:version/coverage or something, you could have it as one of the tabs after 'Builds'.

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.

The general approach seems good :)

@jyn514 jyn514 added A-builds Area: Building the documentation for a crate A-frontend Area: Web frontend C-enhancement Category: This is a new feature S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Aug 6, 2020
@GuillaumeGomez GuillaumeGomez force-pushed the doc-coverage branch 2 times, most recently from 587b863 to f75d0fe Compare August 6, 2020 18:55
@GuillaumeGomez
Copy link
Member Author

Updated! I also added it into the "popup" as @jyn514 said. :) Screenshots updated.

@GuillaumeGomez GuillaumeGomez force-pushed the doc-coverage branch 2 times, most recently from 6eedfc2 to b13ef25 Compare August 7, 2020 18:31
@jyn514 jyn514 added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Aug 7, 2020
@jyn514
Copy link
Member

jyn514 commented Aug 7, 2020

Blocked on rustwide publishing a new version.

@pietroalbini
Copy link
Member

Pushed two commits to this branch, bumping rustwide to 0.10.0 and removing the macro.

@GuillaumeGomez
Copy link
Member Author

🎊

@jyn514 jyn514 added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. labels Aug 11, 2020
@jyn514 jyn514 merged commit 9c55f9b into rust-lang:master Aug 12, 2020
@GuillaumeGomez GuillaumeGomez deleted the doc-coverage branch August 12, 2020 12:27
@sunjay
Copy link
Member

sunjay commented Aug 12, 2020

Hey @GuillaumeGomez, thanks for taking the time to add this feature. :)

I was wondering if we could move the Coverage section to a metrics page or something? I'm not sure if it makes sense to display this so prominently in the crate information dropdown. Documentation coverage is a great thing to know, but it doesn't indicate the quality of the crate or even the quality of its documentation. (It has the same downsides as using code coverage as the only metric for how good your tests are.)

I think having access to this information is good for library authors, but we should really find a proper place for it where we can make sure it's as useful as possible. If we have a separate page or tab or something, we can keep adding more things to it to help library authors while avoiding overpopulating the crate information dropdown.

I think it's great that you added it to the Crate tab as well as the dropdown. Maybe we could remove it from the dropdown for the timebeing while we figure out how to best go about displaying this information? :)

@GuillaumeGomez
Copy link
Member Author

Thanks for the feedback @sunjay!

For now, the feature itself is pretty limited, and we agree: in some cases, the doc coverage isn't an important information (pure FFI bindings for examples). However, for everything else, we've been trying in the documentation team to enforce people to document all items by adding lints (missing_docs and missing_doc_code_examples) to help crates' maintainer in this task. Putting this information forward is just the next step and I really think that it's an important information for anyone that is comparing multiple crates. Documentation quantity isn't an insurance for good documentation, but no documentation is also a good factor.

For now it's only about how many items are documented, but I intend to modify this value by also adding more checks like "how much doc comments are actually providing examples". So this is just a first step, and I am curious to see how it'll be used by users. The "hidden" goal being to force people to document more of course. :3

@yaahc
Copy link
Member

yaahc commented Aug 12, 2020

The "hidden" goal being to force people to document more of course. :3

I agree with this goal but not the approach. This feels like negative re-enforcement to me and I think this will have negative impacts on maintainers emotionally when they're being guilt-tripped into adding docs. I'm not sure it should be removed but I think you should tread carefully here and be sure you've considered the emotional impact of this change.

@GuillaumeGomez
Copy link
Member Author

I wanted to add this information in the crate results directly, but for now I think it's too early for that and that we should let maintainers get to know how it works before putting everything in place. For now, I think the display is quite discrete: when browsing through docs.rs, it's "rare" to take a look at those places, which is why we think it's fine for the moment.

The goal isn't to shame anyone for their work. Everyone should be thankful for the incredible work of all rustaceans. We're just trying to help the crate maintainers by giving them tools to improve their crate even more, and to users to help them to have a crate that matches what they need better. Some crates are not meant to be used by "external" users, which is perfectly fine, but in this case, it's "normal" (still think it's a good thing to have documentation in any case, if not others at least for yourself :p) to have less documentation and it's a good indicator that this crate might not be the one you want.

However, once again: we didn't promote this display more specifically because we want to help people understand and adjust to the system. It'll also allow us to provide even more information and tools for both crate's owners and users.

@pietroalbini
Copy link
Member

Maybe we could make it less prominent?

2020-08-12--21-37-14

@GuillaumeGomez
Copy link
Member Author

@pietroalbini Might be a bit hard to get what it's talking about, no? Not really sure... However, maybe we could move the documentation coverage block under "dependencies" and "versions" blocks. What do you think?

@yaahc
Copy link
Member

yaahc commented Aug 12, 2020

For now, I think the display is quite discrete: when browsing through docs.rs, it's "rare" to take a look at those places, which is why we think it's fine for the moment.

I looked more carefully and you're right, I had mistakenly assumed it was in the sidebar on the landing page for the crate docs when you navigate to something like docs.rs/foo, so things are already not as bad as I'd assumed.

@GuillaumeGomez
Copy link
Member Author

We can add it later on, but for now (and the next months in fact...) it's not scheduled. Next step will be to provide a page to help crates' maintainers to understand how this score is computed.

@pietroalbini
Copy link
Member

Might be a bit hard to get what it's talking about, no?

@GuillaumeGomez tried moving it at the bottom and making the text more clear:

2020-08-12--22-40-58

@GuillaumeGomez
Copy link
Member Author

But still not keeping what the number is about? :p

@pietroalbini
Copy link
Member

I don't think the exact number of items documented is that important. We can make that field a link to the about page though, which already shows the detailed data.

@GuillaumeGomez
Copy link
Member Author

Saying what it is seems quite important from my point of view. But as you prefer...

@pietroalbini
Copy link
Member

Opened #961 with my proposed change

@jyn514 jyn514 removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Sep 20, 2020
@jyn514 jyn514 mentioned this pull request Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builds Area: Building the documentation for a crate A-frontend Area: Web frontend C-enhancement Category: This is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants