Codecov Report

Merging #2827 (c5f2b27) into master (9f7124c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2827   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         136      137    +1     
  Lines       12279    12287    +8     
=======================================
+ Hits        12041    12049    +8     
  Misses        162      162           
  Partials       76       76           

@gmlewis thanks again for the quick feedback! I'll probably commit the requested changes by tomorrow.

one of the reasons for this is that GitHub has a history of adding new options to API endpoints. If we didn't do this, then we would be continually breaking the API of this client library by being forced to update the signature of this method to keep adding new fields.

This makes perfect sense. I should've thought of that, thanks for the heads up!

Just two questions though, as I am still not very familiar with this package's approach to API design. I saw a lot of other methods using pointers to the Options struct containing the body payload. This is good in the sense that it could potentially avoid a copy of a big struct, but it also allows for passing in a nil pointer which could then likely be dereferenced later on and cause a panic or a runtime error in case a payload is required. I take it this is an accepted risk in this package's design?

Additionally, the Options structs themselves will often (but strangely not always) be composed of only reference types. I imagine this is done intentionally in order to distinguish between Go's default initialized values and the presence or absence of values in the option itself. However, this also could introduce a runtime panic if a required parameter is forgotten, is this also by design then?

In order words, despite Action being mandatory for this specify endpoint, I should design my Options struct like this:

type ReviewPersonalAccessTokenRequestOptions struct {
	Action *string `json:"action,omitempty"`
	Reason *string `json:"reason,omitempty"`
}

And not like this:

type ReviewPersonalAccessTokenRequestOptions struct {
	Action string `json:"action"`
	Reason *string `json:"reason,omitempty"` // or string
}

Is my understanding of the above points, correct?

... I saw a lot of other methods using pointers to the Options struct containing the body payload. ... I take it this is an accepted risk in this package's design?

So you will probably see inconsistencies in this regard within this client library, which I will take the blame for.
Ideally, if a method requires body parameters to be provided, then we would simply pass an opts parameter that is a struct passed by value (and not a struct pointer), and what I would recommend doing in this case.

Additionally, the Options structs themselves will often (but strangely not always) be composed of only reference types. ... is this also by design then?

Again, ideally, if a passed parameter is mandatory, we would not use a pointer, and we would not use omitempty in the JSON tag. If a passed parameter is optional, then we use a pointer and include omitempty. There are also a few fiddly cases in the API where we must send a null or other weirdness where we take other measures, but I don't think this particular endpoint has any fiddly cases.

So it appears to me that your last suggestion would be preferred since action is mandatory and reason is not:

type ReviewPersonalAccessTokenRequestOptions struct {
	Action string `json:"action"`
	Reason *string `json:"reason,omitempty"`
}

In case the last comment wasn't clear, I would think this method would have this in it:
opts ReviewPersonalAccessTokenRequestOptions) ...

(in other words, not a pointer to a struct, but instead using pass-by-value.)

@gmlewis Thank you for clarifying!
I've pushed some changes, let me know if you'd like me to change anything else.

Additionally, I've made the parameter requestID a string in the ReviewPersonalAccessTokenRequest method, since it is a path parameter. However, strictly speaking, it is guaranteed by the current API to be an integer (see docs). Do you think I should keep it as a string just in case GitHub changes their mind about this ID later, or switch it to a int64?

Do you think I should keep it as a string just in case GitHub changes their mind about this ID later, or switch it to a int64?

Since the official docs say that it is an "integer", then let's please make it an int64 in the hopes that that will reduce developer confusion. Thanks.

Oh, and you can leave all the "%v" in the fmt.Sprintf as that is idiomatic Go and commonly used throughout this repo. We only change this "%v" to other things when formatting must be non-standard (e.g. "%05d").

Done!

Thank you, @liaodaniel !
Merging.