Support preview Protected Branches API #934
Conversation
googlebot
added
the
cla: yes
| @@ -560,11 +560,13 @@ type RequiredStatusChecks struct { | |||
| // PullRequestReviewsEnforcement represents the pull request reviews enforcement of a protected branch. | |||
| type PullRequestReviewsEnforcement struct { | |||
| // Specifies which users and teams can dismiss pull request reviews. | |||
There was a problem hiding this comment.
As written. the new omitempty tags aren't useful because the fields are not pointers and therefore can not be nil.
But looking back at https://github.com/google/go-github/pull/637/files , I'm not seeing why these were not originally made to be pointers with the omitempty tag. Maybe @shurcooL remembers? I read through the comments but it looks like I didn't bring up this point before and neither did anyone else.
Unless @shurcooL says otherwise, I'm thinking that these fields need to be turned into pointers and the omitempty tags should be added since these are return values.
There was a problem hiding this comment.
I'm sorry I didn't get you. Should I make any changes or are we waiting for @shurcooL for a response?
There was a problem hiding this comment.
Let's go ahead and change these fields to pointers and add omitempty since this struct is part of the return value.
There was a problem hiding this comment.
@gmlewis let me make sure I got It correct, you are suggesting that we add omitempty to the following structs, Right?
There was a problem hiding this comment.
Sorry for the confusion. It looks like you have already added omitempty to all the PullRequestReviewsEnforcement struct fields, which is what I was requesting.
I'm thinking that to make the omitempty tags effective, we should also make all the fields pointers.
So they would be:
DismissalRestrictions *DismissalRestrictions ...
DismissStaleReviews *bool ...
RequireCodeOwnerReviews *bool ...
...Please let me know if that is not clear.
@jkosecki - maybe you can comment on what you think about these proposed changes since you authored #617?
There was a problem hiding this comment.
This may be a noob question but why are we using pointers for types like int and bool? @gmlewis
There was a problem hiding this comment.
That is an excellent question, @scriptonist, and there have been several very good discussions about it. In a nutshell, it is so that we can tell if the server populated the field that it sent back to us which is especially important for variables like bool where you want to know if you are really getting back a false or if the server didn't send a value at all.
It is actually more important on the request side than the response side, really... which is why they may not be needed at all in this situation. I think I'm contradicting what I said earlier, unfortunately, and apologize. I said "since this struct is part of the return value". That is not good enough reason to make this change, really...
So I'm thinking now, let's hold off on my most recent request to make these all pointers.
Especially if the docs seem to indicate that these fields will always be sent in the reply.
Hopefully @jkosecki or @shurcooL can provide a code review and then we can get this merged when they are also happy with the changes. As this stands, I'm going to give this my LGTM.
Thanks again, @scriptonist, for bearing with me.
Here is a search of some of the previous great discussions about using pointers in this repo:
https://github.com/google/go-github/issues?utf8=%E2%9C%93&q=is%3Aissue+pointers
There was a problem hiding this comment.
Thanks, @gmlewis for the detailed explanation. I was curious to know why things are like this. This was a perfect answer to that. I really appreciate your willingness to assist.
|
Thank you, @juliaferraioli! |
|
@gmlewis I was worried that would be the case. Thanks :-\ |
|
Actually, now that I'm looking at the final results of the merge, all the important things have been addressed, and the extra See #974 for details. Closing. |
This PR corresponds to #876
I've removed the protected branches preview media type from the corresponding functions which were required to complete this PR.Please review and suggest fixes if needed, Apologies for the late PR.
Fixes #876