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.

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

Add PR check for documentation coverage #686

Closed
3 tasks
sffc opened this issue Apr 28, 2021 · 10 comments
Closed
3 tasks

Add PR check for documentation coverage #686

sffc opened this issue Apr 28, 2021 · 10 comments
Assignees
Labels
C-process Component: Team processes S-medium Size: Less than a week (larger bug fix or enhancement) T-docs-tests Type: Code change outside core library

Comments

@sffc
Copy link
Member

sffc commented Apr 28, 2021

In the PR template, we should make a checklist of to-do items that aren't covered elsewhere (CI). Suggestions:

  1. Documentation coverage on all new exported types
  2. Adherence to each section of the style guide
  3. Others?

UPDATE: See discussion below. Deliverables:

  • Ensure all exported types are documented
  • Then, enforce the missing_docs check in a new CI job
  • Ensure the CI check returns failures on documentation errors like missing links
@sffc sffc added T-docs-tests Type: Code change outside core library C-process Component: Team processes discuss Discuss at a future ICU4X-SC meeting S-tiny Size: Less than an hour (trivial fixes) labels Apr 28, 2021
@gregtatum
Copy link
Member

Is this motivated by issues that are slipping in? I'd prefer not to complicate the PR process too much if possible. It adds friction to the contribution process.

@sffc
Copy link
Member Author

sffc commented Apr 28, 2021

Is this motivated by issues that are slipping in? I'd prefer not to complicate the PR process too much if possible. It adds friction to the contribution process.

It is motivated by issues such as #681. I see it as a way to supplement CI for things that we cannot programmatically test for. Just like we shouldn't allow failing tests to be merged, we also shouldn't allow undocumented exported types to be merged.

@sffc
Copy link
Member Author

sffc commented Apr 28, 2021

Another way to resolve this issue would be if we could have a CI task that enforced the documentation constraint (failing if it finds any exported type without a docstring).

@gregtatum
Copy link
Member

I would assume that a checklist would work similar to false-positives in the CI, where the checklist quickly becomes something ignored when it's not relevant.

For issue #681, there is the missing_docs lint which could be helpful.

https://doc.rust-lang.org/rustdoc/lints.html

#![allow(missing_docs)] // allows the lint, no diagnostics will be reported
#![warn(missing_docs)] // warn if there are missing docs
#![deny(missing_docs)] // error if there are missing docs

Enabling this for icu_datetime yields 163 warnings.

@zbraniecki
Copy link
Member

Maybe we could use https://doc.rust-lang.org/rustdoc/unstable-features.html#--show-coverage-calculate-the-percentage-of-items-with-documentation (still only in nightly rust-lang/rust#58154 ) to get a sense of coverage and nudge, instead of forcing, our culture in the right way?

@sffc sffc changed the title Add checklist for PRs Add PR check for documentation coverage May 1, 2021
@sffc sffc added S-medium Size: Less than a week (larger bug fix or enhancement) and removed S-tiny Size: Less than an hour (trivial fixes) labels May 1, 2021
@sffc
Copy link
Member Author

sffc commented May 1, 2021

In my opinion, the next steps on this issue should be to run the documentation coverage tool, get it to a point where it passes (either by adding docs or by adding #![allow(missing_docs)]), and then adding the CI check to prevent backsliding.

@sffc
Copy link
Member Author

sffc commented May 9, 2021

This new CI should also include a check that fails for missing links and other errors outputted by cargo doc.

@sffc
Copy link
Member Author

sffc commented May 13, 2021

@gregtatum suggested doing this on a per-module level to split it into smaller pieces and avoid a big PR with bad documentation.

@sffc sffc added this to the 2021 Q2-m3 milestone May 13, 2021
@sffc sffc removed the discuss Discuss at a future ICU4X-SC meeting label May 13, 2021
@sffc
Copy link
Member Author

sffc commented Aug 12, 2021

  • @gregtatum - The scope of filling missing docs is huge.
  • @nordzilla - We could add small # doc comments on APIs that need them, so they are easily searchable
  • @Manishearth - We could do this crate by crate. Empty doc comments seems worse.
  • @sffc - I would say to migrate everything that can be migrated, enable it as a global lint, and disable certain crates, each with issues to track them.

@gregtatum
Copy link
Member

I'm marking this issue as resolved with #955, and the smaller missing docs issues are filed per component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-process Component: Team processes S-medium Size: Less than a week (larger bug fix or enhancement) T-docs-tests Type: Code change outside core library
Projects
None yet
Development

No branches or pull requests

3 participants