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 warning for missing documentation #727

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add warning for missing documentation #727

wants to merge 3 commits into from

Conversation

elegaanz
Copy link
Member

To force us to document our code.

I didn't added it to binaries (src/ and plume-cli/ because it didn't really made sense for them IMO), but I can.

Also, to keep this PR small, I propose that we slowly document each function as we modify them in other PRs. Does it seem reasonable to you?

@elegaanz elegaanz added C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed A: Meta More about project management or code than the project itself Suggestion Proposed ideas worth considering labels Jan 21, 2020
Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall idea sounds reasonable. Considering src/ (maybe plume-cli) also contains some non trivial code (remember url! in the custom domain PR?), I think doing it would make sense to apply that on them too

You should add a feature to all crates, and replace current attribute with #![cfg_attr(not(feature = "ci"), warn(missing_docs))] (which would be set in CI). That way it won't fail on the deny(warnings) on clippy

@igalic
Copy link
Contributor

igalic commented Jan 21, 2020

can't we add that that somewhere to clippy or check?

@elegaanz
Copy link
Member Author

Isn't there a better way to allow just these warnings in clippy?

@codecov
Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Merging #727 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
- Coverage   39.17%   39.15%   -0.03%     
==========================================
  Files          73       73              
  Lines        9653     9653              
  Branches     2183     2182       -1     
==========================================
- Hits         3782     3780       -2     
  Misses       4819     4819              
- Partials     1052     1054       +2

@elegaanz elegaanz changed the base branch from master to main July 5, 2020 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Meta More about project management or code than the project itself C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed Suggestion Proposed ideas worth considering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants