joyent / git-apply-pr

Validate GitHub PullRequests and add metadata
MIT License
8 stars 4 forks source link

reviewed-by: allow +1 next to lgtm #3

Closed robertkowalski closed 9 years ago

robertkowalski commented 9 years ago

use case: in apache projects we usually vote by a +1 and never by a lgtm

if the patch is making the tool unusable for node, I can make it configurable

misterdjules commented 9 years ago

I would like it to be configurable. I'm used to seeing LGTM used in a more "formal" manner to express that a change passes a code review done by the author of the comment. On the other hand, it seems that using "+1" is interpreted in many different ways, including just to express the desire for a feature and not to review code.

That makes me think that it seems git-apply-pr doesn't check if the author of the LGTM or +1 is a collaborator. If so, then it seems we should add that too.

misterdjules commented 9 years ago

@robertkowalski Did you have time to take a look at my comment? Checking if the author of the LGTM comment is a collaborator doesn't need to be done for this PR, we could file a new issue for that.

robertkowalski commented 9 years ago

i got time tonight :)

testing if the person is collaborator sadly does not work for apache where everyone in the org can't push / close issues - as the ASF just mirrors to github.

Our workflow is:

robertkowalski commented 9 years ago

additionally the github api just shows the collaborators for a repo just if you have push access and i could not find a property on the comment-data itself.

robertkowalski commented 9 years ago

@misterdjules updated :)

misterdjules commented 9 years ago

@robertkowalski Minor comments, otherwise LGTM :+1: Thank you very much, and my apologies for such a long delay.

robertkowalski commented 9 years ago

I think we are ready to go @misterdjules

robertkowalski commented 9 years ago

let me just fix the redirects, so we can test :)

misterdjules commented 9 years ago

Landed in d1c00ee7302a797f9fef67878fb35dc841dde51c, thanks!