Document Git.CreateCommit and PullRequests.Edit arguments #539
Conversation
github/pulls.go
Outdated
| @@ -197,6 +197,7 @@ type pullRequestUpdate struct { | |||
| } | |||
|
|
|||
| // Edit a pull request. | |||
| // pull may be nil, in which case the requested PR is returned unmodified. | |||
There was a problem hiding this comment.
I'm trying to think about the best thing to do in a situation where pull is nil.
Have you also considered returning an error, similar to when commit is nil in CreateCommit?
If so, how did you pick which one to go with?
I can see that for the CreateCommit case, there's really no other viable choice but to return error if commit is nil. You can't create a commit if it weren't supplied, and the only reason Commit is being given as a pointer is to copy less memory, not because nil value has any meaning.
For Editing a pull request, it's a little less clear and both options are viable.
My main concern with accepting pull value of nil as a valid operation is that it would encourage doing that in some situations, but it seems silly to make a network request to GitHub API servers with an noop operation, doesn't it? So maybe we should be more aggressive and return error here too, if pull is nil?
Or do you think it makes sense to allow the pull request parameter value of nil? In that case, should we consider not making the network request, or is that "too smart" for what's meant to be more or less a direct wrapper to the GH API?
There was a problem hiding this comment.
Sorry I let this one slip by... Yes, I think you are right.
There really is no need to call Edit with pull == nil since Get could be called instead.
I'll make this an error too.
| Parents: parents, | ||
| } | ||
| if commit.Tree != nil { | ||
| body.Tree = commit.Tree.SHA | ||
| } |
There was a problem hiding this comment.
One question about this new if commit.Tree != nil { statement. Do you know why it wasn't here before, and why did you decide to add it now?
I don't see this change being mentioned in the commit message, and I think it's worth including.
There was a problem hiding this comment.
It looks to me like I saw a potential runtime panic and chose to plug it.
I'll add a comment to the commit message.
| // | ||
| // The commit.Committer is optional and will be filled with the commit.Author | ||
| // data if omitted. If the commit.Author is omitted, it will be filled in with | ||
| // the authenticated user’s information and the current date. | ||
| // | ||
| // GitHub API docs: https://developer.github.com/v3/git/commits/#create-a-commit | ||
| func (s *GitService) CreateCommit(ctx context.Context, owner string, repo string, commit *Commit) (*Commit, *Response, error) { | ||
| if commit == nil { | ||
| return nil, nil, fmt.Errorf("commit must be provided") |
There was a problem hiding this comment.
Given that this func (and PullRequestsService.Edit) is now documented that "commit must not be nil", and it returns an error, we have two choices of what to do if the user uses the API incorrectly and commit is actually nil:
- Panic.
- Return an error (currently being done).
Which of those two options do you think is better @gmlewis any why?
There was a problem hiding this comment.
Effective Go recommends:
... real library functions should avoid panic.
and I strongly agree. I would much rather that my server/program spit out an error message and continue on (or choose to die, depending on its particular circumstances) than to have some library choose for me to kill my program.
There was a problem hiding this comment.
I completely agree that library functions should avoid panic during normal processing.
But that same Effective Go section also says "One possible counterexample is during initialization", and I think another counterexample could be for invalid API usage. For example, consider (crypto/rand).Int function:
// Int returns a uniform random value in [0, max). It panics if max <= 0.
func Int(rand io.Reader, max *big.Int) (n *big.Int, err error) {Panicking (rather than returning error) on incorrect API usage seems totally appropriate, since the goal is to let the user know that they are using the API incorrectly as soon as possible, so the most drastic option (such as panic) is desired.
Most other places in the standard library that panic on invalid API usage don't have an error return value (e.g., (math/big).Int.Sqrt, so returning a errors.New("x must be non-negative") is not even an option there.
However, that is not very applicable in this situation.
The biggest reason why I think we have to go with returning error here in go-github (rather than panicking) is the previous behavior of this method. Previous behavior was to accept nil value for *Commit and not panic (but return an error that comes from GitHub response), so it would be completely unacceptable for us to suddenly change that.
If we were designing a new API from scratch, then panicking on incorrect API usage, or choosing to use a value to make it impossible to misuse the API, is something we could've considered. But we're not.
So 👍 to returning error. :)
Additionally, protect code from potential panic in CreateCommit. Fixes google#531. Change-Id: Ifc6640db138e4ea936f3c3077c1563fe3eedd450
Change-Id: Ibd502b7ab2d8e6bddda834e65529fa7f58141cc3
|
I believe I have addressed all feedback, @shurcooL. |
|
Friendly ping. I'll let you merge it @gmlewis since it's your PR. |
|
Thanks for the ping, @shurcooL! |
Document Git.CreateCommit and PullRequests.Edit arguments Additionally, protect code from potential panic in CreateCommit. Fixes google#531.
This also changes the logic of
Git.CreateCommitto ensure that thecommitargument is non-nil.I am against changing this argument to a value because it is a non-trivial struct and makes sense (to me) to be passed by reference.
Fixes #531.
Change-Id: Ifc6640db138e4ea936f3c3077c1563fe3eedd450