iframe-proxy

anubhakushwaha · 2017-12-23T15:30:00Z

Helps #814 .
The current pull request includes the GraphQL node ID in the response for the following REST API v3 resources:

Deployments
Gists
Git Blobs
Git Commits
Git References
Git Tags
Issues
Labels
Milestones
Organizations

googlebot · 2017-12-23T15:30:09Z

anubhakushwaha · 2017-12-23T15:35:10Z

I signed it!

googlebot · 2017-12-23T15:35:17Z

CLAs look good, thanks!

anubhakushwaha · 2017-12-23T15:39:07Z

@gmlewis @shurcooL I have added GraphQL node ID in the response, let me know if I am doing it correctly and would love to correct and work upon my mistakes.Thanks!

dmitshur

This is a large change, thank you for taking it on @anubhakushwaha!

It's looking great at a high level so far. I've spotted a few minor issues and change suggestions for you to consider, see inline comments.

dmitshur · 2017-12-23T23:58:39Z

@@ -193,6 +209,9 @@ func (s *GistsService) GetRevision(ctx context.Context, id, sha string) (*Gist,
 	if err != nil {
 		return nil, nil, err
 	}


Add a blank line here for consistency.

dmitshur · 2017-12-23T23:58:51Z

+
+		if m := mediaTypeGraphQLNodeIDPreview; m != r.Header.Get("Accept") {
+			t.Errorf("Request accept header = %v, want %v", r.Header.Get("Accept"), m)
+		}


Hmm, why not use testHeader(t, r, "Accept", mediaTypeGraphQLNodeIDPreview) helper here?

@shurcooL While working on this file, I noticed that in git_blobs_test.go and git_trees_test.go we have used:

if m := "GET"; m != r.Method { t.Errorf("Request method = %v, want %v", r.Method, m) }

which is tested differently from the rest of the files for the header method where we have done it like :

testMethod(t, r, "GET")

Is their any specific reason for doing so? I implemented the test on the accept header to maintain the consistency with the respective files. Let me know your thoughts.

@anubhakushwaha IMO, the inconsistency in git_trees_test.go and git_blobs_test.go is by accident rather than having any reasoning behind it. I'll suggest you to use testHeader in your PR to make it consistent with the entire package.

We should instead fix the inconsistency to avoid confusions like this one in future.

@sahildua2305 Thanks for the suggestion, will update the PR.

Are you still planning to do this? Just checking.

dmitshur · 2017-12-24T00:07:15Z

 	GitPushURL  *string                   `json:"git_push_url,omitempty"`
 	CreatedAt   *time.Time                `json:"created_at,omitempty"`
 	UpdatedAt   *time.Time                `json:"updated_at,omitempty"`
+	NodeID	    *string		  `json:"node_id,omitempty"`


This line and many others aren't gofmted. You'll want to use gofmt command to format all these files. You can either invoke it directly in this directory:

gofmt -l -w .

Or you can format the entire package via go's fmt subcommand:

https://golang.org/cmd/go/#hdr-Run_gofmt_on_package_sources

go fmt github.com/google/go-github/github

Thanks @shurcooL Resolved!

dmitshur · 2017-12-24T00:08:19Z


+		if m := mediaTypeGraphQLNodeIDPreview; m != r.Header.Get("Accept") {
+			t.Errorf("Request accept header = %v, want %v", r.Header.Get("Accept"), m)
+		}


Here as well.

Queried above.

dmitshur · 2017-12-24T00:10:23Z

 	mux.HandleFunc("/repos/o/r/git/commits/s", func(w http.ResponseWriter, r *http.Request) {
 		testMethod(t, r, "GET")
-		testHeader(t, r, "Accept", mediaTypeGitSigningPreview)
+		testHeader(t, r, "Accept", strings.Join(acceptHeaders, ", "))


This is minor and either way is fine, but I'll make the suggestion for you to consider.

Since acceptHeaders is only used here, you can declare it right here. No need to declare it that much earlier:

acceptHeaders := []string{mediaTypeGitSigningPreview, mediaTypeGraphQLNodeIDPreview} testHeader(t, r, "Accept", strings.Join(acceptHeaders, ", "))

@shurcooL It was done to maintain consistency with the rest of the files when we had multiple mediaType values. Let me know if I should change it 😄

Okay, keeping it as is is completely acceptable.

dmitshur · 2017-12-24T00:16:56Z

Also, you should edit the PR description and replace "Partially fixes #814." with something like "Helps #814." because GitHub syntax doesn't recognize the word "partially". It only sees "fixes #814", so it'll close that issue when this is merged, which isn't what you want.

kshitij10496 · 2017-12-24T09:01:45Z

The build is failing due to formatting issues with the code.

Run $ gofmt -d -s .in a terminal from the base directory of the repo.
This will show the diff of all formatting issues with the code.
I think fixing them should resolve the build failure.

anubhakushwaha · 2017-12-24T10:16:35Z

@kshitij10496 Thanks, I will keep it in mind from future 😄

anubhakushwaha · 2017-12-24T10:19:09Z

 	}

+	// TODO: remove custom Accept header when this API fully launches.
+	req.Header.Set("Accept", mediaTypeGraphQLNodeIDPreview)


One of the reasons of failing Travis, forgot to add it previously.

dmitshur

A few minor comments. Otherwise this is looking good, not seeing other issues or opportunities for improvement.

It's hard to verify definitively which exact objects get the new NodeID field. I tried to confirm, and it looks good, but I can't give definitive guarantees. For example, I'm not quite sure if DeploymentStatus should get NodeID (it probably should, but I'm not 100% confident). GitHub documentation doesn't make it very easy to know for sure, so I think this is as good as it can be for the first version.

dmitshur · 2018-01-05T18:22:43Z

 	// Pulls.ListCommits, Repositories.ListCommits, etc.
-	CommentCount *int `json:"comment_count,omitempty"`
+	CommentCount *int    `json:"comment_count,omitempty"`
+	NodeID       *string `json:"node_id,omitempty"`


I think it'd be nicer to either move NodeID field up, together with all other fields, or add a blank line after CommentCount, since it has a comment saying that "This is only populated for requests that fetch GitHub data", which doesn't apply to NodeID as I understand.

Minor, but worth doing.

dmitshur · 2018-01-05T18:24:12Z

+
+		if m := mediaTypeGraphQLNodeIDPreview; m != r.Header.Get("Accept") {
+			t.Errorf("Request accept header = %v, want %v", r.Header.Get("Accept"), m)
+		}


Are you still planning to do this? Just checking.

dmitshur · 2018-01-05T18:28:16Z

 	// TextMatches is only populated from search results that request text matches
 	// See: search.go and https://developer.github.com/v3/search/#text-match-metadata
 	TextMatches []TextMatch `json:"text_matches,omitempty"`
+	NodeID      *string     `json:"node_id,omitempty"`


Also here, move NodeID field up to be with all other fields, rather than with TextMatches.

gmlewis

Very nice, @anubhakushwaha... thank you!
Thanks to @shurcooL and @sahildua2305 for the code reviews, too!

After you address @shurcooL's review requests, this LGTM.

anubhakushwaha · 2018-01-06T07:00:02Z

@shurcooL @sahildua2305 Apologies for the delay 😄 Updated the PR. Please review.

sahildua2305

LGTM

dmitshur

LGTM; just see one minor thing that needs to be done before we merge this.

dmitshur · 2018-01-09T21:27:49Z

@@ -1,4 +1,4 @@
-// Copyright 2017 The go-github AUTHORS. All rights reserved.
+// Copyright 2018 The go-github AUTHORS. All rights reserved.


You'll need to revert this change. See #823 for the reason why. Sorry about the trouble.

I've applied this trivial change.

@shurcooL It was my pleasure working on this 😄

See google#823 for rationale.

googlebot · 2018-01-09T21:31:19Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

dmitshur · 2018-01-09T21:35:51Z

Merged. Thanks a lot for working on this @anubhakushwaha!

anubhakushwaha · 2018-01-10T07:51:51Z

Add NodeID field to objects that now expose it. Reference: https://developer.github.com/changes/2017-12-19-graphql-node-id/.

googlebot added the cla: no label Dec 23, 2017

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Dec 23, 2017

anubhakushwaha force-pushed the graphQL branch 3 times, most recently from 4e56d34 to 0fe4ed7 Compare December 23, 2017 16:50

dmitshur requested changes Dec 24, 2017

View reviewed changes

anubhakushwaha force-pushed the graphQL branch 4 times, most recently from 46b1c2e to 6e4b5f4 Compare December 24, 2017 08:32

anubhakushwaha mentioned this pull request Dec 24, 2017

Migrate IDs to int64 type #816

Merged

4 tasks

anubhakushwaha force-pushed the graphQL branch from 6e4b5f4 to ec3fd21 Compare December 24, 2017 10:13

anubhakushwaha commented Dec 24, 2017

View reviewed changes

anubhakushwaha force-pushed the graphQL branch from ec3fd21 to 5f06315 Compare December 24, 2017 10:21

dmitshur requested a review from gmlewis January 5, 2018 18:20

dmitshur requested changes Jan 5, 2018

View reviewed changes

gmlewis approved these changes Jan 5, 2018

View reviewed changes

anubhakushwaha added 5 commits January 6, 2018 12:21

Added a new media-type in github.go

b6bc94a

Added a new media-type and node_id in repos_deployment.go

7eff1e9

Updated repos_deployments_test.go

ca11286

Changed gists.go and gists_test.go

67dad31

Changed git_blobs.go and git_blobs_test.go

d172ce0

anubhakushwaha added 9 commits January 6, 2018 12:21

Changed git_commits.go and git_commits_test.go

39a4cbf

Changed git_refs.go and git_refs_test.go

6eb915f

Changed git_tags.go and git_tags_test.go

f783b7d

Changed issues.go and issues_test.go

64e67ad

Changed issues_labels.go and issues_labels_test.go

a8a4153

Changed issues_milestones.go and issues_milestones_test.go

6ed5b44

Changed orgs.go and orgs_test.go

7d4a9c4

Fixed travis fail

a22ba94

Moved NodeID with other fields + Made accept headers consistent

4fa822b

anubhakushwaha force-pushed the graphQL branch from 5f06315 to 4fa822b Compare January 6, 2018 06:55

sahildua2305 approved these changes Jan 8, 2018

View reviewed changes

dmitshur approved these changes Jan 9, 2018

View reviewed changes

Continue to use 2017 as the copyright year in github-accessors.go.

3eab45d

See google#823 for rationale.

googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Jan 9, 2018

dmitshur merged commit cbeb72e into google:master Jan 9, 2018

sahildua2305 mentioned this pull request Jan 12, 2018

Make use of testHeader and testMethod consistent #829

Closed

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

Support preview GraphQL API v4 Node IDs. (google#817)

2c66f4a

Add NodeID field to objects that now expose it. Reference: https://developer.github.com/changes/2017-12-19-graphql-node-id/.

palash25 mentioned this pull request Dec 30, 2018

Support preview GraphQL API v4 Node IDs in REST API v3 resources #814

Closed

		@@ -1,4 +1,4 @@
		// Copyright 2017 The go-github AUTHORS. All rights reserved.
		// Copyright 2018 The go-github AUTHORS. All rights reserved.

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

anubhakushwaha commented Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Dec 23, 2017

Uh oh!

anubhakushwaha commented Dec 23, 2017

Uh oh!

googlebot commented Dec 23, 2017

Uh oh!

anubhakushwaha commented Dec 23, 2017

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anubhakushwaha Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kshitij10496 commented Dec 24, 2017

Uh oh!

anubhakushwaha commented Dec 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

anubhakushwaha commented Jan 6, 2018

Uh oh!

sahildua2305 left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anubhakushwaha commented Dec 23, 2017 •

edited

Loading

anubhakushwaha Dec 24, 2017 •

edited

Loading

dmitshur commented Dec 24, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading