Skip to content

Extend coverage feature #1072

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 2 commits into from
Oct 1, 2020

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Sep 30, 2020

Like we talked about some time ago, the doc examples have been added to rustdoc so we can now use them on docs.rs as well!

For information, I decided to go for this computation: the documentation itself is worth 80% of the rating, the doc examples is the remaining 20%.

After debate, for now it won't be taken into account in the percentage computation.

Here is what it looks like:

Screenshot from 2020-10-01 16-06-23

@jyn514 jyn514 added A-builds Area: Building the documentation for a crate C-enhancement Category: This is a new feature S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Sep 30, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 30, 2020

Hmm, I'm not sure we should count them as the same percentage - most people don't even know missing_doc_examples exists. Maybe we could have two separate percentages, and make doc-examples a smaller font or something?

@jyn514
Copy link
Member

jyn514 commented Sep 30, 2020

cc @sunjay and @yaahc who had concerns on #939

@sunjay
Copy link
Member

sunjay commented Sep 30, 2020

Hmm, I'm not sure we should count them as the same percentage - most people don't even know missing_doc_examples exists. Maybe we could have two separate percentages, and make doc-examples a smaller font or something?

Yeah I'm also not sure about having them as the same percentage. If that ends up being the way we do it, we should at least include an asterisk somewhere to tell people how it's calculated. Don't want to cause unnecessary confusion.

@Manishearth
Copy link
Member

I still share @sunjay's concerns from #939 (comment) about this feature, but less so because it's not as prominent anymore.

I'm quite wary of coming up with an arbitrary formula for this. What's the reasoning behind it? Why 80%? And yeah we should be transparent about it, or perhaps just include two different numbers?

@GuillaumeGomez
Copy link
Member Author

I'm quite wary of coming up with an arbitrary formula for this. What's the reasoning behind it? Why 80%? And yeah we should be transparent about it, or perhaps just include two different numbers?

I agree, we should at least put the explanation of the computation somewhere. Just don't know where... As for why not two numbers: because the ultimate goal is to provide this information alongside crates search results (when you look for crates on docs.rs, not when you search something in a crate).

@pietroalbini
Copy link
Member

I wouldn't include examples in the percentage, as for a lot of items examples are just noise. I think it's fine to display that number, but it shouldn't count for the %.

@GuillaumeGomez
Copy link
Member Author

I wouldn't include examples in the percentage, as for a lot of items examples are just noise

Interesting argument! Do you have some examples in mind by any chance?

@pietroalbini
Copy link
Member

I think a big example is builders (like Command in Rustwide), where you have a bunch of fn set_option(self, new_value: bool) -> Self that don't really have any meaningful examples (but still need text documentation to explain what the option does).

@GuillaumeGomez
Copy link
Member Author

The example in itself isn't useful but:

  1. It doesn't hurt to have it
  2. It allows to copy/paste code directly

So it is definitely worth it having examples even then in my opinion.

@pietroalbini
Copy link
Member

I feel like useless examples are just noise: if a crate author wants to add them that's fine, but we shouldn't encourage people to do so.

@GuillaumeGomez
Copy link
Member Author

Unfortunately, we can't have shades here: it's either we enforce it on everything (there are still items that aren't required by the lint and those are fines) or on nothing... Methods, types and functions should always be documented and have a code example.

@Nemo157
Copy link
Member

Nemo157 commented Oct 1, 2020

I agree that having an example on every item should not be necessary for the overall quality indicator. As another example I have this fully public error type which is shown in other examples. Adding an example on each variant (judging by the percentage shown public fields are not required to have examples?) would be useless extra detail clogging up the documentation. I personally consider this crate to be nearly 100% covered with examples (less adding some examples for the single item Alphabet), but --show-coverage gives it just a 47% or 54% rating depending on features.

@GuillaumeGomez
Copy link
Member Author

You're missing my point (also, at least one short example should be on the type directly with a link to the examples, but that's another debate): we can relax the rules later on, but currently, most crates are strongly lacking documentation. For some of them, it's fine (FFI ones), but it's problematic for all others.

@Nemo157
Copy link
Member

Nemo157 commented Oct 1, 2020

at least one short example should be on the type directly with a link to the examples

I very much disagree with the first half of this sentence (but the latter is definitely something I should add 😁).

Actually testing adding empty examples to stuff it appears I was wrong about the variants being the source of low coverage. The problem is that I don't have examples on the types and instead link out to the more centralized documentation. (There does seem like 1 bug here that type aliases probably shouldn't be in the set of items that require examples?).

@GuillaumeGomez
Copy link
Member Author

It's open to debate. The option is still nightly (and will remain for some time I expect), so you can definitely open an issue on the rust repository so it can be discussed. But yes, fields and variants are excluded from this count. ;)

@jyn514
Copy link
Member

jyn514 commented Oct 1, 2020

I think that especially since this option is experimental and changing rapidly, we should not put it front and center on the /crate page. I'd be fine with listing the number of examples:
image

But not with showing the percentage:
image

And certainly not by including the examples in the 'overall documentation' percentage:

image

@GuillaumeGomez does that first option sound reasonable to you?

@GuillaumeGomez
Copy link
Member Author

I'm not too strongly in favor of merging both results in the computed percentage, even though I think we should. For now we can simply show the numbers. The important part for me is to have the data available so we can use it later on. If it's fine with you, I'll then keep the display of the example numbers (not the percentage) and remove it from the total percentage computation.

@jyn514
Copy link
Member

jyn514 commented Oct 1, 2020

If it's fine with you, I'll then keep the display of the example numbers (not the percentage) and remove it from the total percentage computation.

Sounds like a plan :)

@jyn514 jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 1, 2020
@GuillaumeGomez GuillaumeGomez force-pushed the extend-coverage-feature branch from ef99082 to a44e6a9 Compare October 1, 2020 14:07
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez GuillaumeGomez force-pushed the extend-coverage-feature branch 2 times, most recently from 04ef20b to e4fc371 Compare October 1, 2020 14:32
@Manishearth
Copy link
Member

Manishearth commented Oct 1, 2020

As for why not two numbers: because the ultimate goal is to provide this information alongside crates search results (when you look for crates on docs.rs, not when you search something in a crate).

I am very strongly against this, for the reasons @sunjay articulated in #939 (comment) (also @yaahc's comment in #939 (comment)). I do not think we should engineer a race-for-metrics via some coverage number.

I was under the impression that based on discussion in the other thread this was no longer the goal.

I've already heard people complaining about flaws in lib.rs' ranking algorithm, and of crates sometimes doing things that don't improve them to improve their "score"

If this is the direction we're going in I'd like to see a holistic plan (instead of incremental changes), and, ideally, an RFC.

@sunjay
Copy link
Member

sunjay commented Oct 1, 2020

I strongly agree with what Manish said. I am also very against putting this in the search results for every crate.

A proposal I could get on board with:

  • Make this into a badge that people can add to their README and/or Cargo.toml
  • Display badges in the search results

That way people can have the metric and choose where they want it to be displayed. It's a win-win.

Overall, I don't think this is a bad feature. It makes sense to provide metrics like this just like it makes sense to provide people with the ability to generate code coverage reports. I just think that people shouldn't be forced into having to care about a number that they might not care about and that doesn't even accurately represent the quality of their crate. Putting this number in the search results would definitely be overstating its importance.

I think Manish put it perfectly when he said:

I do not think we should engineer a race-for-metrics via some coverage number.

I completely agree with this.

@GuillaumeGomez
Copy link
Member Author

This isn't supposed to impact ranking, just a visual indication alongside the crate.

@Manishearth
Copy link
Member

This isn't supposed to impact ranking, just a visual indication alongside the crate.

Yes, I'm aware, but when it's prominent in search results people will still optimize for it, and this creates negative reinforcement. We should avoid negative reinforcement, regardless of the desired outcome being a good thing. I want people to write more docs. I do not wish to sneakily browbeat them into doing it by setting up such incentives. It's not respectful of the community, and this kind of thing rarely works the way it was intended to.

There already are people going around bugging maintainers into add #![forbid(unsafe)] to their crates just because cargo-geiger considers this a good thing. This is for a third party tool, and I've already seen a bunch of people annoyed by this. It will be much worse if docs.rs starts doing such things prominently.

@GuillaumeGomez
Copy link
Member Author

Hum, I see... I'm shared on this. I mean: it sounds good to have more documentation overall. However, you bring up a good concerning point: this is an easy-to-trick metric and people will very likely optimize for it. This is really sad. :-/

I didn't want to put this information alongside a crate's name before long, but maybe it's best to never do it even if it makes me sad... However, it can still be very interesting metrics to use for us. It could be useful to have extra information to help people improve their crate documentation when you have a download/documentation coverage ratio information. But this is all very far future.

Anyway, thanks for bringing this up: this is definitely something where we need to be very careful.

As for the PR, is there anything else I need to do?

@Manishearth
Copy link
Member

I didn't want to put this information alongside a crate's name before long, but maybe it's best to never do it even if it makes me sad... However, it can still be very interesting metrics to use for us. It could be useful to have extra information to help people improve their crate documentation when you have a download/documentation coverage ratio information. But this is all very far future.

I think having it on the crate info page in docs.rs (where it is right now) is fine. It's not immediately prominent, and I think ultimately such a number is useful, we just don't want to surface it so prominently that people start overindexing on it.

I think the current PR, where the examples are not included in the calculation, is fine. Worried about people getting confused but we can figure out precise styling later too, if this is actually a problem.

@GuillaumeGomez GuillaumeGomez force-pushed the extend-coverage-feature branch from e4fc371 to 2213af1 Compare October 1, 2020 15:59
@GuillaumeGomez GuillaumeGomez force-pushed the extend-coverage-feature branch from 2213af1 to 693a0ea Compare October 1, 2020 16:06
@jyn514 jyn514 merged commit 38ba774 into rust-lang:master Oct 1, 2020
@GuillaumeGomez GuillaumeGomez deleted the extend-coverage-feature branch October 1, 2020 17:54
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 C-enhancement Category: This is a new feature S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants