Skip to content

Conversation

@gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Jun 15, 2016

Fixes #278.

Change-Id: Ia1d0c1f6b395d493714d91e0f6e79dc2506f8fde

@dmitshur
Copy link
Member

The actual content in this PR looks good to me, I don't see any issues. LGTM.

I do have 2 style comments that I'll leave inline for your discretion. The first one (writing gofmted Go code, even if it's inside a comment) is highly recommended. The second one is completely optional and up to you.

github/users.go Outdated
// }
// opts.Since = *users[len(users)-1].ID
// // Process users...
// }
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest making this Go code gofmted. Right now it uses 1 space indent, followed by 2 spaces. How about this:

https://gist.githubusercontent.com/shurcooL/43a5922765e1918ae00f29f315e9b41c/raw/e1de820beeece8b3a6a881e8f0d1913901e7c71c/comment.go

(Note that the comment always begins with 2 slashes and a space. The rest is its content. The code section is indented by 1 tab. This is the most idiomatic and consistent way to write Go comments, at least if you're as picky about them as me.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very nice. Thanks, @shurcooL!

Fixes google#278.

Change-Id: Ia1d0c1f6b395d493714d91e0f6e79dc2506f8fde
@gmlewis gmlewis merged commit 71e65bb into google:master Jun 16, 2016
@gmlewis gmlewis deleted the i-278-since branch June 16, 2016 18:27
gmlewis added a commit to gmlewis/go-github that referenced this pull request Jun 16, 2016
See google#367 for discussion.

Change-Id: I10e26bddebeea2a1485fc4389caab1ff12421382
gmlewis added a commit to gmlewis/go-github that referenced this pull request Jun 16, 2016
See google#367 for discussion.

Change-Id: I10e26bddebeea2a1485fc4389caab1ff12421382
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants