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

Deny missing docs #563

Closed
killercup opened this Issue Jan 1, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@killercup
Copy link
Member

killercup commented Jan 1, 2017

Diesel should have a thoroughly documented API, and I'd like it to compile with #[deny(missing_docs)]. To make this even harder better, I'd like it to have an example for each major¹ public item.

What needs documenting

  • Here's a list² of the items that currently miss documentation: https://gist.github.com/killercup/baf6975ea40d70cd278823a6af9bc05d
  • Many items already have doc comments and thus don't show up in this list, but they could use some more extensive explanations or examples, so feel free to make PRs for those as well.
  • Module-level documentation can always use more examples
  • Other good documentation PRs would be adding a CONTRIBUTING.md, and README.md files for all crate subfolders to (shortly) describe their purpose. (This will also make contributing to this issue easier!)

If you want to help

A good size for pull requests is "one pull request per file". This will reduce git churn and it is in general easier to review short PRs.

I would suggest doing the following:

  1. Clone this repository
  2. Set up your system so the tests run without errors, feel free to ask on Gitter if you have problems
  3. Choose a file/module/item you want to add documentation to, and add #![deny(missing_docs)] to it (or #[deny(missing_docs)] for items like structs or traits)
  4. Write nice documentation in easy to understand but precise English
  • Read the official book's chapter on documentation if you haven't yet
  • Feel free to follow these guidelines for formatting.
  • If you add code examples, make sure they compile and run, just like test. Have a look at existing doc tests or ask on Gitter if you need help.
  1. Ensure your code compiles with newly added documentation.
  2. Open a PR on this repo and refer to this issue

¹ definition of 'major public item' pending, suggestions welcome 😄
² generated with (get rq with cargo install record-query)

cargo +nightly rustc --features 'postgres sqlite chrono unstable' \
  -- -D missing_docs --error-format json 2>&1 | \
  grep '^{' | \
  rq -jH 'filter [level, error] |
    flatMap spans |
    map (s) => { {file: `${s.file_name}:${s.line_start}`, text: s.text[0].text.trim()} } |
    uniqBy file |
    map (x) => { `${x.file} ${x.text}` }'
@mattjmcnaughton

This comment has been minimized.

Copy link

mattjmcnaughton commented Jan 6, 2017

@killercup I'd love to take a stab at this if possible (first commit in Rust)! I've modified the code to include the #![deny(missing_docs)] and am happy to submit a pr for that. Were you also envisioning this diff fixing all of those items missing documentation as well?

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Jan 6, 2017

first commit in Rust

Wow, that's amazing, @mattjmcnaughton!

Adding the deny itself will just make it not compile any more until all items in that list have doc comments. So we'll have to wait a bit before we can add this. :) Locally, you could also use #[warn(missing_docs)] to let the compiler give you a warning for all the undocumented public items in the code base (basically my list).

That's a lot of items to write documentation for, so I it makes sense to have a lot of smaller PRs adding docs for singular modules or traits.

Oh, and I totally forgot to link to these guidelines to keep the doc comment style consistent. (Not that I want each doc comment as extensive as the examples in there, though! Please let me know if you have suggestions to improve these guidelines.)

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Jan 6, 2017

Were you also envisioning this diff fixing all of those items missing documentation as well?

Definitely don't submit one PR fixing all of the documented items. One PR per file is about the right size. We can't accept a PR that adds #![deny(missing_docs)] until after the documented items are fixed.

@mattjmcnaughton

This comment has been minimized.

Copy link

mattjmcnaughton commented Jan 7, 2017

Sounds like a plan (and thank you for the quick response)! As I understand the code, and as I get the change, I'll make some separate prs with the documentation changes, and when the documented items are fixed, I'll make a pr that adds #![deny(missing_docs)].

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Jan 7, 2017

@stuartellis

This comment has been minimized.

Copy link

stuartellis commented Jan 9, 2017

I'm new to Rust, but I'd be happy to help with this.

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Jan 9, 2017

I just added a few more details on how contribute to the issue description: #563 (comment)

@stuartellis

This comment has been minimized.

Copy link

stuartellis commented Jan 9, 2017

@killercup - Thank you for that.

@mattjmcnaughton - FWIW, I think that I'll start with just CONTRIBUTING.md and README.md files before trying anything more in-depth myself.

@stuartellis

This comment has been minimized.

Copy link

stuartellis commented Jan 19, 2017

Just to say that I am not going to be able to work on this, unfortunately (too many projects, something has to give).

@ericho

This comment has been minimized.

Copy link
Contributor

ericho commented Jan 26, 2017

Sent a very simple (and probably wrong) PR #592

@killercup

This comment has been minimized.

Copy link
Member

killercup commented Jan 26, 2017

@killercup killercup added this to API docs in Documentation Feb 12, 2017

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Dec 16, 2017

Y'all need to review my open PRs so I can close this. ;P

@sgrif sgrif referenced this issue Dec 19, 2017

Merged

Docs 🎉 #1419

@sgrif sgrif closed this in #1419 Dec 20, 2017

@sgrif sgrif removed this from API docs in Documentation Jan 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment