Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove preview media type for Team Review Requests #844

Merged
merged 1 commit into from Feb 23, 2018

Conversation

kshitij10496
Copy link
Contributor

@kshitij10496 kshitij10496 commented Feb 5, 2018

As the Team Review Requests API is fully supported by GitHub API v3, we
have removed the preview(custom) media type: thor-preview.

Fixes #835


  1. How should we proceed here given that some GitHub enterprise versions may not yet support the Team Review Requests API?
  2. According to the new versioning guideline, under which category should be removing preview functionality in favor of GitHub API support fall?

Ping @willnorris @gmlewis @sahildua2305

As the Team Review Requests API is fully supported by GitHub API v3, we
have removed the preview(custom) media type: thor-preview.

Closes google#840
@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Feb 5, 2018
@gmlewis
Copy link
Collaborator

gmlewis commented Feb 5, 2018

Re: question 1: GitHub Enterprise customers must now pin specific tagged releases of this package and we can move forward without hesitation due to #376.

Re: question 2: According to our new versioning strategy, the tagged minor version would be bumped in this case.

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 5, 2018

In your first comment above, you wrote Closes #840 - but did you mean "Fixes #835" instead?

@kshitij10496
Copy link
Contributor Author

In your first comment above, you wrote Closes #840 - but did you mean "Fixes #835" instead?

Yes, I meant to refer #835 instead of #840. Thanks for pointing this out.
I will be more mindful while writing the commit message.
I think I should rectify the commit message accordingly and force push the change, shouldn't I?

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 5, 2018

I think you can simply edit that first message. A force push should not be necessary because I can make sure the commit message has the right text when merging... as long as I remember to do that correctly. 😄

@kshitij10496
Copy link
Contributor Author

Sure! That's awesome.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Thank you, @kshitij10496!
I'll let this sit for a day in case there are any other comments, then merge tomorrow (unless another reviewer LGTM's and merges before then). Feel free to ping me if I forget to merge tomorrow.

Copy link
Member

@sahildua2305 sahildua2305 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

@kshitij10496
Copy link
Contributor Author

I'll let this sit for a day in case there are any other comments, then merge tomorrow (unless another reviewer LGTM's and merges before then). Feel free to ping me if I forget to merge tomorrow.

This makes sense.
How about we formulate a guideline to merge any change(as trivial as this PR) after at least 24 hours have passed from the moment of the patch being sent, so as to give everyone time to review the work? I remember @shurcooL raising a related point in a comment. Thus, I think we can mention this formally in the Contributing document.

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 5, 2018

I think it is a good idea in general to wait for comments/conversation on a PR, but am not sure that we need to formalize it.

Sometimes there are good reasons to merge things ASAP... I think it might keep things a bit more flexible if we leave it up to the maintainers of the repo to decide on a PR-by-PR basis.

@gmlewis
Copy link
Collaborator

gmlewis commented Feb 23, 2018

Sorry for the delay... Merging.

@gmlewis gmlewis merged commit b165831 into google:master Feb 23, 2018
gmlewis added a commit that referenced this pull request Feb 24, 2018
nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018
As the Team Review Requests API is fully supported by GitHub API v3, we
have removed the preview(custom) media type: thor-preview.

Fixes google#840.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants