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

Last set of doc comments #121

Merged
merged 1 commit into from Jul 31, 2014

Conversation

Projects
None yet
3 participants
@bharrisau
Contributor

bharrisau commented Jul 30, 2014

closes #82

@@ -52,6 +52,7 @@ pub struct C12332<'a, S, T, P> {
}
impl<'a, S: SPI, T: Timer, P: GPIO> C12332<'a, S, T, P> {
/// Create a new C12332.

This comment has been minimized.

@farcaller

farcaller Jul 30, 2014

Member

"Creates a new C12332 driver instance"?

@@ -13,6 +13,19 @@
// See the License for the specific language governing permissions and
// limitations under the License.
//! Definition of a 9x9 font.
//!
//! Font definition is consists of a 4 byte header:

This comment has been minimized.

@farcaller

This comment has been minimized.

@bharrisau

bharrisau Jul 30, 2014

Contributor

Yes, I should probably double check my own work or you'll start to think I'm lazy.

This comment has been minimized.

@farcaller

farcaller Jul 30, 2014

Member

Taking in the account the number of commits you did in the last 7 days and I did in the last 7 days, who's lazy? ;) keep up the great work.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 30, 2014

r?

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 30, 2014

I might have misread a few previous PRs (or I just had seen too much Go code in the last few hours), but didn't we use something like 'xxx creates a new C12332 driver instance' (where we do omit xxx most of the times) as compared to e.g. 'Set an individual pixel.'?

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 30, 2014

I haven't been doing that form for any of my doc comments.

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 30, 2014

Ok, I checked over my own code and I do not follow the consistent pattern in there (apart from 'returns' which I always write that way).

Rust API docs also use same, er... syntax. I think it's worth to unify that one.

@bharrisau

This comment has been minimized.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 30, 2014

I just need 2-3 examples so I know what it should look like.

@farcaller

This comment has been minimized.

Member

farcaller commented Jul 30, 2014

I'm now reading through rust docs and guess what, it's all inconsistent as well.

Thankfully our guide includes the great description:

Every function declaration should have comments immediately preceding it that describe what the function does and how to use it. These comments should be descriptive ("Opens the file") rather than imperative ("Open the file"); the comment describes the function, it does not tell the function what to do. In general, these comments do not describe how the function performs its task. Instead, that should be left to comments in the function definition.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 30, 2014

Clear enough. I can't say I'll fix everything up, but I'll have a look
through the drivers stuff and update this.

@bharrisau

This comment has been minimized.

Contributor

bharrisau commented Jul 31, 2014

Ok. I'm pretty happy with this now.

@farcaller

This comment has been minimized.

farcaller commented on 1b54285 Jul 31, 2014

r+

@hacknbot

This comment has been minimized.

Contributor

hacknbot commented on 1b54285 Jul 31, 2014

saw approval from farcaller
at bharrisau@1b54285

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

merging bharrisau/zinc/doc3 = 1b54285 into auto

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

bharrisau/zinc/doc3 = 1b54285 merged ok, testing candidate = 5bf3fc63

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

Merge sha 2f0c71b is stale.

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

merging bharrisau/zinc/doc3 = 1b54285 into auto

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

merging bharrisau/zinc/doc3 = 1b54285 into auto

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

bharrisau/zinc/doc3 = 1b54285 merged ok, testing candidate = 352af6a

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

Merge sha 1f62cb8 is stale.

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

merging bharrisau/zinc/doc3 = 1b54285 into auto

This comment has been minimized.

Contributor

hacknbot replied Jul 31, 2014

bharrisau/zinc/doc3 = 1b54285 merged ok, testing candidate = 9050979

hacknbot added a commit that referenced this pull request Jul 31, 2014

Merge pull request #121 from bharrisau/doc3
Last set of doc comments

Reviewed-by: farcaller

hacknbot added a commit that referenced this pull request Jul 31, 2014

Merge pull request #121 from bharrisau/doc3
Last set of doc comments

Reviewed-by: farcaller

hacknbot added a commit that referenced this pull request Jul 31, 2014

Merge pull request #121 from bharrisau/doc3
Last set of doc comments

Reviewed-by: farcaller

hacknbot added a commit that referenced this pull request Jul 31, 2014

Merge pull request #121 from bharrisau/doc3
Last set of doc comments

Reviewed-by: farcaller

farcaller added a commit that referenced this pull request Jul 31, 2014

Merge pull request #121 from bharrisau/doc3
Last set of doc comments

@farcaller farcaller merged commit 5ab4d8d into hackndev:master Jul 31, 2014

1 of 2 checks passed

default running tests for candidate 9050979ae1f11c3ab6d32af0efa7bda6ec3a15a5
Details
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment