Remove custom media type for global node IDs #925

S2606 · 2018-06-06T04:53:15Z

Fixes #919

googlebot · 2018-06-06T04:53:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

S2606 · 2018-06-06T05:03:41Z

I signed it!

googlebot · 2018-06-06T05:03:44Z

CLAs look good, thanks!

S2606 · 2018-06-06T05:04:10Z

@gmlewis Please review

gmlewis

Thank you, @S2606!

Instead of commenting out the unwanted lines, please go ahead and delete them so that we don't have old comments left lying around... as they would just cause confusion when someone new to the repo started reading and contributing to the code.

Check out https://github.com/google/go-github/pull/844/files for an example of removal.

Thanks again!

S2606 · 2018-06-08T04:48:10Z

@gmlewis Changes have been made.

gmlewis

Thank you, @S2606!

This is looking better. There are more opportunities to clean up the code if you don't mind.

gmlewis · 2018-06-09T01:50:19Z

github/git_commits.go

@@ -70,7 +70,7 @@ func (s *GitService) GetCommit(ctx context.Context, owner string, repo string, s
 	}

 	// TODO: remove custom Accept headers when APIs fully launch.
-	acceptHeaders := []string{mediaTypeGitSigningPreview, mediaTypeGraphQLNodeIDPreview}
+	acceptHeaders := []string{mediaTypeGitSigningPreview}
 	req.Header.Set("Accept", strings.Join(acceptHeaders, ", "))


Let's please simplify this one:

req.Header.Set("Accept", mediaTypeGitSigningPreview)

gmlewis · 2018-06-09T01:51:13Z

github/git_commits_test.go

@@ -19,7 +19,7 @@ func TestGitService_GetCommit(t *testing.T) {
 	client, mux, _, teardown := setup()
 	defer teardown()

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


Same here. No need for the acceptHeaders slice anymore.

gmlewis · 2018-06-09T01:51:46Z

github/git_tags.go

@@ -45,7 +45,7 @@ func (s *GitService) GetTag(ctx context.Context, owner string, repo string, sha
 	}

 	// TODO: remove custom Accept headers when APIs fully launch.
-	acceptHeaders := []string{mediaTypeGitSigningPreview, mediaTypeGraphQLNodeIDPreview}
+	acceptHeaders := []string{mediaTypeGitSigningPreview}
 	req.Header.Set("Accept", strings.Join(acceptHeaders, ", "))


gmlewis · 2018-06-09T01:52:09Z

github/git_tags_test.go

@@ -19,7 +19,7 @@ func TestGitService_GetTag(t *testing.T) {
 	client, mux, _, teardown := setup()
 	defer teardown()

-	acceptHeaders := []string{mediaTypeGitSigningPreview, mediaTypeGraphQLNodeIDPreview}
+	acceptHeaders := []string{mediaTypeGitSigningPreview}
 	mux.HandleFunc("/repos/o/r/git/tags/s", func(w http.ResponseWriter, r *http.Request) {
 		testMethod(t, r, "GET")
 		testHeader(t, r, "Accept", strings.Join(acceptHeaders, ", "))


gmlewis · 2018-06-09T01:52:40Z

github/issues.go

@@ -270,7 +270,7 @@ func (s *IssuesService) Create(ctx context.Context, owner string, repo string, i
 	}

 	// TODO: remove custom Accept header when this API fully launches.
-	acceptHeaders := []string{mediaTypeGraphQLNodeIDPreview, mediaTypeLabelDescriptionSearchPreview}
+	acceptHeaders := []string{mediaTypeLabelDescriptionSearchPreview}
 	req.Header.Set("Accept", strings.Join(acceptHeaders, ", "))


gmlewis · 2018-06-09T01:57:31Z

github/issues_labels_test.go

@@ -83,7 +83,7 @@ func TestIssuesService_CreateLabel(t *testing.T) {

 	input := &Label{Name: String("n")}

-	acceptHeaders := []string{mediaTypeGraphQLNodeIDPreview, mediaTypeLabelDescriptionSearchPreview}
+	acceptHeaders := []string{mediaTypeLabelDescriptionSearchPreview}


Same here for line 92 below.

gmlewis · 2018-06-09T01:58:23Z

github/issues_labels_test.go

@@ -122,7 +122,7 @@ func TestIssuesService_EditLabel(t *testing.T) {

 	input := &Label{Name: String("z")}

-	acceptHeaders := []string{mediaTypeGraphQLNodeIDPreview, mediaTypeLabelDescriptionSearchPreview}
+	acceptHeaders := []string{mediaTypeLabelDescriptionSearchPreview}


Same here for line 131 below.

gmlewis · 2018-06-09T01:58:41Z

github/issues_labels_test.go

@@ -181,7 +181,7 @@ func TestIssuesService_ListLabelsByIssue(t *testing.T) {
 	client, mux, _, teardown := setup()
 	defer teardown()

-	acceptHeaders := []string{mediaTypeGraphQLNodeIDPreview, mediaTypeLabelDescriptionSearchPreview}
+	acceptHeaders := []string{mediaTypeLabelDescriptionSearchPreview}
 	mux.HandleFunc("/repos/o/r/issues/1/labels", func(w http.ResponseWriter, r *http.Request) {
 		testMethod(t, r, "GET")
 		testHeader(t, r, "Accept", strings.Join(acceptHeaders, ", "))


gmlewis · 2018-06-09T01:59:10Z

github/issues_labels_test.go

@@ -218,7 +218,7 @@ func TestIssuesService_AddLabelsToIssue(t *testing.T) {

 	input := []string{"a", "b"}

-	acceptHeaders := []string{mediaTypeGraphQLNodeIDPreview, mediaTypeLabelDescriptionSearchPreview}
+	acceptHeaders := []string{mediaTypeLabelDescriptionSearchPreview}


Same here for line 227 below.

gmlewis · 2018-06-09T01:59:57Z

github/issues_labels_test.go

@@ -280,7 +280,7 @@ func TestIssuesService_ReplaceLabelsForIssue(t *testing.T) {

 	input := []string{"a", "b"}

-	acceptHeader := []string{mediaTypeGraphQLNodeIDPreview, mediaTypeLabelDescriptionSearchPreview}
+	acceptHeader := []string{mediaTypeLabelDescriptionSearchPreview}


You get the idea, I'm going to stop commenting... but here and below as well.

S2606 · 2018-06-10T04:33:31Z

@gmlewis Requested changes have been made.Please review

gmlewis

Thank you... this LGTM.
Awaiting second LGTM before merging.

S2606 · 2018-06-10T12:58:06Z

@shurcooL Please review

S2606 · 2018-06-14T01:22:56Z

@gmlewis Till when can we wait for this PR to be merged?

gmlewis · 2018-06-14T02:04:32Z

We have a policy for this repo to not merge non-trivial PRs until we get at least two LGTMs.
Sometimes it takes a few days to a couple weeks depending upon how busy the other contributors are.
Thank you for your patience! It is greatly appreciated.

gmlewis · 2018-08-10T15:23:00Z

Merging without second LGTM. Sorry for the delay, @S2606!

S2606 · 2018-08-10T16:14:09Z

No problem and thank you for merging. A start for lot more PR's

Fixes google#919.

googlebot added the cla: no label Jun 6, 2018

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

gmlewis requested changes Jun 8, 2018

View reviewed changes

S2606 force-pushed the remove_custom_mediatype branch from 3b0b32e to 0801a40 Compare June 8, 2018 04:47

S2606 force-pushed the remove_custom_mediatype branch from 0801a40 to d2e5883 Compare June 8, 2018 04:53

gmlewis requested changes Jun 9, 2018

View reviewed changes

Remove custom media type for global node IDs

67a036f

S2606 force-pushed the remove_custom_mediatype branch from d2e5883 to 67a036f Compare June 10, 2018 04:31

gmlewis approved these changes Jun 10, 2018

View reviewed changes

Merge branch 'master' into remove_custom_mediatype

9ae4ab5

gmlewis merged commit 29a3681 into google:master Aug 10, 2018

gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Aug 10, 2018

Remove custom media type for global node IDs (google#925)

89b460f

Fixes google#919.

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Remove custom media type for global node IDs (google#925)

89561b5

Fixes google#919.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove custom media type for global node IDs #925

Remove custom media type for global node IDs #925

S2606 commented Jun 6, 2018

googlebot commented Jun 6, 2018

S2606 commented Jun 6, 2018

googlebot commented Jun 6, 2018

S2606 commented Jun 6, 2018

gmlewis left a comment

S2606 commented Jun 8, 2018

gmlewis left a comment

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

gmlewis Jun 9, 2018

S2606 commented Jun 10, 2018

gmlewis left a comment

S2606 commented Jun 10, 2018

S2606 commented Jun 14, 2018 •

edited

Loading

gmlewis commented Jun 14, 2018

gmlewis commented Aug 10, 2018

S2606 commented Aug 10, 2018

Remove custom media type for global node IDs #925

Remove custom media type for global node IDs #925

Conversation

S2606 commented Jun 6, 2018

googlebot commented Jun 6, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

S2606 commented Jun 6, 2018

googlebot commented Jun 6, 2018

S2606 commented Jun 6, 2018

gmlewis left a comment

Choose a reason for hiding this comment

S2606 commented Jun 8, 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

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

S2606 commented Jun 10, 2018

gmlewis left a comment

Choose a reason for hiding this comment

S2606 commented Jun 10, 2018

S2606 commented Jun 14, 2018 • edited Loading

gmlewis commented Jun 14, 2018

gmlewis commented Aug 10, 2018

S2606 commented Aug 10, 2018

S2606 commented Jun 14, 2018 •

edited

Loading