Add GitService.GetBlobRaw. #891

dsymonds · 2018-04-11T00:33:41Z

This addresses #658.

The name isn't great (I would have preferred GetRawBlob), but it matches the previously added RepositoriesService.GetCommitRaw. Happy to switch if you'd prefer the better name and don't mind the mild inconsistency.

gmlewis

LGTM.
Thank you, @dsymonds!
Awaiting second LGTM before merging.

gmlewis · 2018-04-11T00:45:26Z

github/git_blobs.go

+	if err != nil {
+		return nil, err
+	}
+	req.Header.Set("Accept", "application/vnd.github.v3.raw")


Do you want to make "application/vnd.github.v3.raw" a const in github.go?
No big deal... either way is fine with me.

I considered it, but it didn't seem necessary for this initial case. If any other code starts using it then that's probably a good enough time to define it as a constant.

dmitshur · 2018-04-11T01:53:07Z

I was hoping that this would follow the same pattern as in the previous PRs #481 and #767, but I see now that the choice of supported media types by the blob endpoints is not the same.

According to https://developer.github.com/v3/media/#commits-commit-comparison-and-pull-requests:

The Commit, Commit Comparison, and Pull Request resources support diff and patch formats:

diff

patch

sha

(It's missing JSON, but it is also supported.)

But the blob endpoint are described at https://developer.github.com/v3/media/#git-blob-properties:

The following media types are allowed when getting a blob:

JSON

raw

I was going to suggest we have a opt RawOptions parameter here like in others, but this complicates things.

The reason to have a parameter like that is in case GitHub adds additional media types. If there's a opt RawOptions-like parameter, it's possible to add support without requiring breaking API changes.

Blob endpoints currently supports only 1 non-JSON raw type, so not having an option works today. It feels like this won't change soon, but it's hard to predict with certainty...

If you think the current approach makes sense, I'm okay with this API (no RawOptions-like parameter), with the hope it will last for some time without needing to change. But it's not an easy call trying to predict how it'll play out.

dmitshur

Thanks for the PR. Here are some comments on how I think we can improve this. Let me know what you think about them.

dmitshur · 2018-04-11T01:54:59Z

github/git_blobs.go

+	req.Header.Set("Accept", "application/vnd.github.v3.raw")
+
+	var buf bytes.Buffer
+	if _, err = s.client.Do(ctx, req, &buf); err != nil {


I would suggest having _, err = s.client.Do(ctx, req, &buf) be on a separate line, like this:

_, err = s.client.Do(ctx, req, &buf) if err != nil { ... }

Making it inline in the if statement makes it look less important, and it's not consistent with the style in the hundreds of other endpoints we have. I think it's a good idea to try to keep the code in this library as consistent as possible, and not deviate unnecessarily. Inconsistent style makes it harder for new contributors to figure out which style to use.

seems like unnecessary consistency, but sure.

dmitshur · 2018-04-11T01:55:39Z

github/git_blobs.go

@@ -38,6 +39,23 @@ func (s *GitService) GetBlob(ctx context.Context, owner string, repo string, sha
 	return blob, resp, err
 }

+// GetBlobRaw fetches a blob's contents from a repo.
+// Unlike GetBlob, it returns the raw bytes rather than the base64-encoded data.


Please add a GitHub API link. It's the same as the one above:

// // GitHub API docs: https://developer.github.com/v3/git/blobs/#get-a-blob

dmitshur · 2018-04-11T01:57:07Z

github/git_blobs.go

@@ -38,6 +39,23 @@ func (s *GitService) GetBlob(ctx context.Context, owner string, repo string, sha
 	return blob, resp, err
 }

+// GetBlobRaw fetches a blob's contents from a repo.
+// Unlike GetBlob, it returns the raw bytes rather than the base64-encoded data.
+func (s *GitService) GetBlobRaw(ctx context.Context, owner, repo, sha1 string) ([]byte, error) {


This method doesn't return *Response. Is there a specific reason you left it out?

Unless there's a reason for it, I believe it's needed for consistency and for callers to be able to find out more details about the response.

I don't know what one would do with the Response apart from what is in the []byte and error return values, but sure.

It might be helpful, for example, to log the response status code from GitHub API in case an error happens.

It's mostly for consistency. All other endpoints return it, so users would expect to be able to access the *github.Response struct in this case too.

dmitshur · 2018-04-11T01:59:32Z

github/git_blobs.go

@@ -38,6 +39,23 @@ func (s *GitService) GetBlob(ctx context.Context, owner string, repo string, sha
 	return blob, resp, err
 }

+// GetBlobRaw fetches a blob's contents from a repo.
+// Unlike GetBlob, it returns the raw bytes rather than the base64-encoded data.
+func (s *GitService) GetBlobRaw(ctx context.Context, owner, repo, sha1 string) ([]byte, error) {


The endpoint above, and the GitHub API documentation (https://developer.github.com/v3/git/blobs/#get-a-blob) uses sha as the name of the parameter (rather than sha1). I suspect it's better if we stay consistent.

gmlewis · 2018-04-11T02:04:27Z

I get the impression that GitHub is focused more upon their GraphQL API for the long-term from the way they were talking at their San Francisco get-together last year... (but that certainly doesn't mean this won't change). So I think the current approach makes sense.

dmitshur · 2018-04-11T02:07:17Z

Yes, that's the main reason I'm okay going without an RawType-like enum parameter. It's hard to justify asking for it now, when it would have a single Raw value, and it feels like there would never be a second value.

dsymonds · 2018-04-11T02:31:55Z

I think I addressed everything. PTAL.

dmitshur

LGTM.

dmitshur · 2018-04-11T04:37:11Z

github/git_blobs_test.go

+
+	blob, _, err := client.Git.GetBlobRaw(context.Background(), "o", "r", "s")
+	if err != nil {
+		t.Errorf("Git.GetBlob returned error: %v", err)


Minor, should be GetBlobRaw in the error text.

whoops. fixed.

Fixes #658.

dmitshur · 2018-04-11T04:53:16Z

Thank you!

dsymonds · 2018-04-11T04:59:21Z

Cool. I'm just glad to upstream some of my custom code. ;-)

This method is similar to GitService.GetBlob, except it uses the "application/vnd.github.v3.raw" media type to fetch the blob content as raw bytes (rather than a JSON object containing a base64-encoded string and other information). GitHub API docs: - https://developer.github.com/v3/git/blobs/#get-a-blob - https://developer.github.com/v3/git/blobs/#custom-media-types Fixes google#658.

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

gmlewis approved these changes Apr 11, 2018

View reviewed changes

dmitshur reviewed Apr 11, 2018

View reviewed changes

dmitshur approved these changes Apr 11, 2018

View reviewed changes

dmitshur reviewed Apr 11, 2018

View reviewed changes

Add GitService.GetBlobRaw.

d311283

Fixes #658.

dmitshur merged commit 89a446e into google:master Apr 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitService.GetBlobRaw. #891

Add GitService.GetBlobRaw. #891

dsymonds commented Apr 11, 2018

gmlewis left a comment

gmlewis Apr 11, 2018

dsymonds Apr 11, 2018

dmitshur commented Apr 11, 2018

dmitshur left a comment •

edited

Loading

dmitshur Apr 11, 2018

dsymonds Apr 11, 2018

dmitshur Apr 11, 2018

dsymonds Apr 11, 2018

dmitshur Apr 11, 2018

dsymonds Apr 11, 2018

dmitshur Apr 11, 2018

dmitshur Apr 11, 2018

dsymonds Apr 11, 2018

gmlewis commented Apr 11, 2018

dmitshur commented Apr 11, 2018 •

edited

Loading

dsymonds commented Apr 11, 2018

dmitshur left a comment •

edited

Loading

dmitshur Apr 11, 2018

dsymonds Apr 11, 2018

dmitshur commented Apr 11, 2018

dsymonds commented Apr 11, 2018

Add GitService.GetBlobRaw. #891

Add GitService.GetBlobRaw. #891

Conversation

dsymonds commented Apr 11, 2018

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur commented Apr 11, 2018

dmitshur left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis commented Apr 11, 2018

dmitshur commented Apr 11, 2018 • edited Loading

dsymonds commented Apr 11, 2018

dmitshur left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur commented Apr 11, 2018

dsymonds commented Apr 11, 2018

dmitshur left a comment •

edited

Loading

dmitshur commented Apr 11, 2018 •

edited

Loading

dmitshur left a comment •

edited

Loading