Added support for preview Lock Reason (#828) #910

sandeepsukhani · 2018-05-08T09:21:52Z

Added preview media type
Added ActiveLockReason fields to Issue and PullRequest responses
Added LockReason(optional) to the IssuesService.Lock request
Added LockReason field to IssueEvent
Added and updated test cases.

Done these changes based on responses that I received using github APIs.
Following were the APIs that I checked:

https://github.com/api/repos/sandlis/test/pulls/2
https://github.com/api/repos/sandlis/test/pulls
https://github.com/api/repos/sandlis/test/events
https://github.com/api/repos/sandlis/test/issues/1
https://github.com/api/repos/sandlis/test/issues
https://github.com/api/repos/sandlis/test/issues/events

1. Added preview media type 2. Added ActiveLockReason fields to Issue and PullRequest responses 3. Added LockReason(optional) to the IssuesService.Lock request 4. Added LockReason field to IssueEvent 5. Added and updated test cases.

gmlewis

This is looking great, @sandlis... thank you!
Just a few minor comments to address if you don't mind.

gmlewis · 2018-05-08T13:03:57Z

github/issues.go

@@ -56,6 +56,8 @@ type Issue struct {
 	// TextMatches is only populated from search results that request text matches
 	// See: search.go and https://developer.github.com/v3/search/#text-match-metadata
 	TextMatches []TextMatch `json:"text_matches,omitempty"`
+
+	ActiveLockReason *string `json:"active_lock_reason,omitempty"`


It would be nice to add a descriptive comment here and enumerate the possible values.

gmlewis · 2018-05-08T13:07:55Z

github/issues_events.go

 	//       The Actor did that to the issue.
 	//
+	//     locked
+	//       The Actor locked the issue.
+	//       lock_reason holds reason of locking the issue if provided while locking


nit: Let's refer to the Go version of the field here (and make it a complete sentence for the godocs):
LockReason holds the reason of locking the issue (if provided while locking).

gmlewis · 2018-05-08T13:08:48Z

github/issues.go

+type LockIssueOptions struct {
+	// LockReason specifies the reason to lock this issue.
+	// Providing a lock reason can help make it clearer to contributors why an issue
+	// was locked. Possible values are: off-topic, too heated, resolved, spam


nit: add quotes for clarity and end in period:
Possible values are: "off-topic", "too heated", "resolved", and "spam".

gmlewis · 2018-05-08T13:10:43Z

github/issues_test.go

+
+	mux.HandleFunc("/repos/o/r/issues/1/lock", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "PUT")
+


Can you please add a testHeader here?
That makes it easier to maintain so that when the header is later removed, it is clear that this needs cleaning up here as well.

gmlewis · 2018-05-08T13:11:42Z

github/pulls.go

@@ -62,6 +62,8 @@ type PullRequest struct {

 	Head *PullRequestBranch `json:"head,omitempty"`
 	Base *PullRequestBranch `json:"base,omitempty"`
+
+	ActiveLockReason *string `json:"active_lock_reason,omitempty"`


Could you please add a descriptive comment here as well with the possible values?

…quested in code review.

sandeepsukhani · 2018-05-08T13:47:58Z

@gmlewis
Thanks for the review!
I am done with requested changes.
Please review and let me know whether it looks good.

gmlewis

LGTM after a couple tiny nits.
Thank you, @sandlis!
Awaiting second LGTM before merging.

gmlewis · 2018-05-08T13:59:56Z

github/issues.go

@@ -56,6 +56,10 @@ type Issue struct {
 	// TextMatches is only populated from search results that request text matches
 	// See: search.go and https://developer.github.com/v3/search/#text-match-metadata
 	TextMatches []TextMatch `json:"text_matches,omitempty"`
+
+	// ActiveLockReason is populated only when LockReason is provided while locking the issue


nit: add period ('.') after issue.

gmlewis · 2018-05-08T14:02:46Z

github/pulls.go

@@ -62,6 +62,10 @@ type PullRequest struct {

 	Head *PullRequestBranch `json:"head,omitempty"`
 	Base *PullRequestBranch `json:"base,omitempty"`
+
+	// ActiveLockReason is populated only when LockReason is provided while locking the pull request


nit: add trailing period after request.

sandeepsukhani · 2018-05-08T14:12:31Z

@gmlewis Added period ('.') at mentioned places.
Thanks!

sandeepsukhani · 2018-05-10T07:25:02Z

@gmlewis
Do I need to invite someone for reviewing this or it gets picked up by mentors themselves as per their availability?
Sorry, this is my second contribution to this project, so I do not know how it works.
In my last PR, a request was sent for 2nd LGTM.

gmlewis · 2018-05-10T11:47:48Z

Hi @sandlis, I just created a new NeedsReview label to inform contributors that they are welcome to review this PR. We rely on volunteers and sometimes PRs sit for a while before one of us can get to it. 😊

sandeepsukhani · 2018-05-10T12:13:21Z

@gmlewis
That should be helpful. Thanks!

gmlewis · 2018-08-10T15:28:50Z

Sorry for the delay, @sandlis! I'm going to attempt to resolve the conflicts and merge this PR.

googlebot · 2018-08-10T15:46:42Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Fixes google#828.

To integrate with go-github api changes that breaks the compiling: - google/go-github#910 - google/go-github#972 Change-Id: I5597f50b9c44fe42acec1128617a25e1fdd09e10 GitHub-Last-Rev: 07754f9 GitHub-Pull-Request: #11 Reviewed-on: https://go-review.googlesource.com/129075 Reviewed-by: Dmitri Shuralyov <[email protected]>

Fixes google#828.

Added support for preview Lock Reason (google#828)

9c38930

1. Added preview media type 2. Added ActiveLockReason fields to Issue and PullRequest responses 3. Added LockReason(optional) to the IssuesService.Lock request 4. Added LockReason field to IssueEvent 5. Added and updated test cases.

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label May 8, 2018

gmlewis requested changes May 8, 2018

View reviewed changes

Updates in comments and 'TestIssuesService_LockWithReason' test as re…

6bce475

…quested in code review.

gmlewis approved these changes May 8, 2018

View reviewed changes

Updates in comments for ActiveLockReason in Issue and PullRequest struct

4a2672a

gmlewis added the NeedsReview PR is awaiting a review before merging. label May 10, 2018

gmlewis added 2 commits August 10, 2018 11:37

Merge branch 'master' into preview_lock_reason

ae727c4

Fix import problem caused during online merge

69652c7

googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Aug 10, 2018

Fix gofmt problem caused by using online GitHub editor

04fbef8

gmlewis merged commit aeb5071 into google:master Aug 10, 2018

gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Aug 10, 2018

Added support for preview Lock Reason (google#828) (google#910)

b4d3f32

Fixes google#828.

h8liu mentioned this pull request Aug 16, 2018

cmd/gopherbot, cmd/gopherstats: update the use of github api golang/build#11

Closed

gmlewis mentioned this pull request Aug 29, 2018

Fixes wrong Accept media type that was causing a 500 #977

Merged

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Added support for preview Lock Reason (google#828) (google#910)

2bdf2df

Fixes google#828.

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Mar 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for preview Lock Reason (#828) #910

Added support for preview Lock Reason (#828) #910

sandeepsukhani commented May 8, 2018

gmlewis left a comment

gmlewis May 8, 2018

gmlewis May 8, 2018

gmlewis May 8, 2018

gmlewis May 8, 2018

gmlewis May 8, 2018

sandeepsukhani commented May 8, 2018

gmlewis left a comment

gmlewis May 8, 2018

gmlewis May 8, 2018

sandeepsukhani commented May 8, 2018

sandeepsukhani commented May 10, 2018

gmlewis commented May 10, 2018

sandeepsukhani commented May 10, 2018

gmlewis commented Aug 10, 2018

googlebot commented Aug 10, 2018


		mux.HandleFunc("/repos/o/r/issues/1/lock", func(w http.ResponseWriter, r *http.Request) {
		testMethod(t, r, "PUT")

Added support for preview Lock Reason (#828) #910

Added support for preview Lock Reason (#828) #910

Conversation

sandeepsukhani commented May 8, 2018

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis May 8, 2018

Choose a reason for hiding this comment

gmlewis May 8, 2018

Choose a reason for hiding this comment

gmlewis May 8, 2018

Choose a reason for hiding this comment

gmlewis May 8, 2018

Choose a reason for hiding this comment

gmlewis May 8, 2018

Choose a reason for hiding this comment

sandeepsukhani commented May 8, 2018

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis May 8, 2018

Choose a reason for hiding this comment

gmlewis May 8, 2018

Choose a reason for hiding this comment

sandeepsukhani commented May 8, 2018

sandeepsukhani commented May 10, 2018

gmlewis commented May 10, 2018

sandeepsukhani commented May 10, 2018

gmlewis commented Aug 10, 2018

googlebot commented Aug 10, 2018