Return http.Response from DownloadContents #1542
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1542 +/- ##
==========================================
+ Coverage 67.90% 67.92% +0.02%
==========================================
Files 94 94
Lines 8670 8670
==========================================
+ Hits 5887 5889 +2
+ Misses 1882 1880 -2
Partials 901 901
Continue to review full report at Codecov.
|
github/repos_contents.go
Outdated
| dir := path.Dir(filepath) | ||
| filename := path.Base(filepath) | ||
| _, dirContents, _, err := s.GetContents(ctx, owner, repo, dir, opts) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err |
There was a problem hiding this comment.
Since s.GetContents returns the response with its third return value, let's go ahead and return it here.
_, dirContents, resp, err := s.GetContents(ctx, owner, repo, dir, opts)
if err != nil {
return nil, resp, err
There was a problem hiding this comment.
The response from s.GetContents is of type *github.Response rather than *http.Response. Do you think there is any value in returning just the http.Response from the github.Response?
We could also consider adding a 4th value to the return to allow the caller to check the rate limits from s.GetContents regardless of other errors?
There was a problem hiding this comment.
Ah! Gotcha. Hmmm... the vast majority of calls in this repo return *github.Response... is it a simple matter to do the same for this endpoint? That would be ideal so that this endpoint would not be a glaring exception.
I'm thinking you can wrap the regular *http.Response just by calling newResponse (around line 438 in github.go) and returning the result.
There was a problem hiding this comment.
Sure, that looks doable. Would it make sense to return the *github.Response from the s.GetContents call with its *http.Response swapped out with the one from requesting the download url? I'm not very familiar with what callers are expecting from the github.Response but that would at least expose the rate limit information from s.GetContents as part of the return value.
This also makes me think it'd be useful to have a method like RepositoriesService.GetDownloadURL(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (string, *github.Response, error) that just returns the URL and leaves it up to the caller to make a request for the contents.
There was a problem hiding this comment.
Hmmm... I would return the most recent response because the point of this PR is to return the most recent response code... so I don't think we want to return anything but the response from the most recent http call, if that makes sense.
There was a problem hiding this comment.
Ok, made the change. I worry a little that this looks consistent by returning github.Response but may not behave consistently. The call to get the download url contents isn't an api call so we aren't able to populate any additional fields on the github.Response like the remaining rate limits.
There was a problem hiding this comment.
Right, but I think this is probably the best option, so let's try it out. If it causes problems in practice, we can make another breaking API change later. 😄
But honestly, I like this best because the other calls that might fail could also be caused by a rate limit or some other issue that we are now exposing back to the caller.
github/repos_contents.go
Outdated
| } | ||
| for _, contents := range dirContents { | ||
| if *contents.Name == filename { | ||
| if contents.DownloadURL == nil || *contents.DownloadURL == "" { | ||
| return nil, fmt.Errorf("No download link found for %s", filepath) | ||
| return nil, nil, fmt.Errorf("No download link found for %s", filepath) |
There was a problem hiding this comment.
Same here...
return nil, resp, fmt.Errorf...
github/repos_contents.go
Outdated
| } | ||
| } | ||
| return nil, fmt.Errorf("No file named %s found in %s", filename, dir) | ||
| return nil, nil, fmt.Errorf("No file named %s found in %s", filename, dir) |
There was a problem hiding this comment.
Same here.
return nil, resp, fmt.Errorf...
github/repos_contents.go
Outdated
| // is from a successful response. | ||
| func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, *http.Response, error) { | ||
| // It is possible for the download to result in a failed response when the | ||
| // returned error is nil. Callers should check the returned http.Response |
There was a problem hiding this comment.
http.Response => Response
|
Ping @wesleimp - thank you! |
|
Thank you, @wesleimp ! Note that this is a breaking API change. |
This is the breaking change we discussed at: #1531 (comment)
Fixes #1531