Skip to content

Add ability to handle comments that ask for review #68

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

Merged
merged 2 commits into from
Feb 9, 2017

Conversation

teldosas
Copy link
Contributor

@teldosas teldosas commented Jan 30, 2017

Address #16

@@ -191,6 +193,8 @@ trait GithubApiActions extends GithubJsonProtocol with core.HttpClient {
def pullRequestCommits(nb: Int) = p[List[Commit]] (Get(api("pulls" / nb / "commits")
withQuery Map("per_page" -> "100")))
def deletePRComment(id: String) = px (Delete(api("pulls" / "comments" / id)))
def requestReview(nb: Int, reviewers: Reviewers) = px (Post(api("pulls" / nb / "requested_reviewers"), reviewers)~>
addHeader("Accept", "application/vnd.github.black-cat-preview+json"))
Copy link
Member

Choose a reason for hiding this comment

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

curious what this is about

Copy link
Contributor Author

@teldosas teldosas Feb 9, 2017

Choose a reason for hiding this comment

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

It's because the PR review API is in Early Access period see here

Copy link
Member

Choose a reason for hiding this comment

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

ah, thanks. I'd suggest commenting this and including the link, since otherwise it will be mysterious to any future maintainer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened #71 for this

@SethTisue
Copy link
Member

interesting... looks plausible... README.md would need an update... have you been able to test this?

@SethTisue
Copy link
Member

nice to see some PRs coming in in this repo, btw. welcome

@teldosas
Copy link
Contributor Author

teldosas commented Feb 9, 2017

have you been able to test this?

Yes I tested it in my fork of Scala

@SethTisue
Copy link
Member

Yes I tested it in my fork of Scala

very cool.

@adriaanm what do you think, shall we just go ahead and merge this and #69 and try them out?

@SethTisue
Copy link
Member

thought: what happens if the requested reviewer doesn't actually have write access to the repo...?

@teldosas
Copy link
Contributor Author

teldosas commented Feb 9, 2017

Well the api request fails for sure, and a message is logged in the console about the user not being a collaborator. I'm not sure if it has any further side-effects. I don't think it would have. I will try it out again soon and I'll get back to you.

@teldosas
Copy link
Contributor Author

teldosas commented Feb 9, 2017

Should I make it post a comment or something in that case?

@SethTisue
Copy link
Member

hmmm... I suppose it's fine if it silently fails (except for the log message).

@adriaanm
Copy link
Contributor

adriaanm commented Feb 9, 2017

Awesome, thank you!! Yes, let's try this out.

@adriaanm adriaanm merged commit 790b634 into scala:master Feb 9, 2017
@SethTisue
Copy link
Member

(note that I still don't know how to deploy a new Scabot, because scala/scala-jenkins-infra#54, and I don't think the backup method is documented anywhere)

@adriaanm
Copy link
Contributor

adriaanm commented Feb 9, 2017

The travis job for the merge commit of this PR seems to have pushed to prod. Checking if post-receive hooks worked (will continue over at the infra ticket).

@adriaanm
Copy link
Contributor

adriaanm commented Feb 9, 2017

It seems to have worked at scala/scala#5662! I couldn't trigger a review at scala/scala#5677 myself by commenting. Not sure what's going on, as I've broken logging :-( while trying to get push-to-deploy? Will get back to this asap

@teldosas
Copy link
Contributor Author

teldosas commented Feb 9, 2017

@adriaanm I think it is not possible for the commiter to be a reviewer

@SethTisue
Copy link
Member

ah, the PR author.

I review-requested myself just now at scala/scala#5677 and it worked fine.

@adriaanm
Copy link
Contributor

adriaanm commented Feb 9, 2017 via email

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