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.

Sign up for GitHub

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

Jump to bottom

v0.20 - Probably Australian #1377

Merged
merged 5 commits into from
Jun 14, 2016
Merged

v0.20 - Probably Australian #1377

merged 5 commits into from
Jun 14, 2016

Conversation

shiftkey
Copy link
Member

@shiftkey shiftkey commented Jun 14, 2016 •
edited
Loading

  • release notes
  • version bump
  • integration tests all pass
  • 👍 from @haacked or @ryangribble
  • tag and publish to NuGet
  • :shipit:

@shiftkey shiftkey force-pushed the release branch 3 times, most recently from cdd7af6 to 80421d5 Compare June 14, 2016 01:37
shiftkey
shiftkey reviewed Jun 14, 2016
View reviewed changes
Octokit.Tests.Integration/Clients/RepositoryHooksClientTests.cs
@@ -276,7 +276,7 @@ public TheDeleteMethod(RepositoriesHooksFixture fixture)
await github.Repository.Hooks.Delete(_fixture.RepositoryOwner, _fixture.RepositoryName, _fixture.ExpectedHook.Id);
var hooks = await github.Repository.Hooks.GetAll(_fixture.RepositoryOwner, _fixture.RepositoryName);

Assert.Empty(hooks);
Assert.Equal(4, hooks.Count);
Copy link
Member Author

Choose a reason for hiding this comment

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

Each test creates the same five hooks, so we need to verify the count of the list to be N-1

shiftkey
shiftkey reviewed Jun 14, 2016
View reviewed changes
Octokit.Tests.Integration/Clients/RepositoryContentsClientTests.cs
@@ -192,7 +192,7 @@ public class TheGetContentsMethod
var archive = await github
.Repository
.Content
.GetArchive("alfhenrik", "ScriptCs.OctoKit", ArchiveFormat.Tarball, "dev");
Copy link
Member Author

Choose a reason for hiding this comment

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

This branch was removed on the repository 😢

Copy link
Contributor

@hnrkndrssn hnrkndrssn Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

💩 sorry! 😇

Copy link
Member Author

Choose a reason for hiding this comment

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

@alfhenrik all good, once I realised what was happening it was an easy fix 😀

@shiftkey
Copy link
Member Author

Pending any feedback on the words from @haacked or @ryangribble this is good to go.

Aiming to do it early Wedneday AEST so I have the day to monitor it in case something unexpected occurs...

@ryangribble
Copy link
Contributor

ryangribble commented Jun 14, 2016 •
edited
Loading

Pending any feedback on the words

  • "Add Migrations API for Enterprise instances".
    This is a bit ambiguous as the API does work on github.com and GHE. Also this is a preview API so may as well mention that, since it's mentioned on various other items.
    Suggest rewording to "Add Migrations Preview API" or "Add Migrations Preview API for migrating repositories to GitHub Enterprise"
  • The details on IUserKeysClient.GetAll -> GetAllForCurrent() #1139 has via ryangribble but should be via @M-Zuber
  • Looking at IUserKeysClient.GetAll -> GetAllForCurrent() #1139 again I think we also may have made a breaking change (renaming GetAll() to GetALlForCurrent without obsoleting the old metho?)
  • I also recall us having other changes where we [Obsoleted] stuff but I can't track them down at the moment!

haacked
haacked reviewed Jun 14, 2016
View reviewed changes
ReleaseNotes.md

**Features**

Pagination support has been added, letting you control how much data you want
Copy link
Contributor

Choose a reason for hiding this comment

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

"has been added" is passive voice and kind of redundant. How about

Pagination support lets you control how much ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, after looking down, I see that "Pagination Support" is being called out in a special manner and isn't in the bulleted list. Maybe we should call out why. For example:

The big focus of this release is pagination support. Pagination support lets you control...

@haacked
Copy link
Contributor

haacked commented Jun 14, 2016

Just a couple minor nits on the release notes. Otherwise, this release is exciting!

@shiftkey
Copy link
Member Author

@haacked @ryangribble thanks for the feedback, I'll incorporate that once I'm done with breakfast.

@shiftkey
Copy link
Member Author

@ryangribble I'll do another pass over the PRs merged to see if I can find other notable [Obsolete] changes - I think #1224 is relevant here

@shiftkey
Copy link
Member Author

🚢ed

@shiftkey shiftkey merged commit a15a3f2 into master Jun 14, 2016
@shiftkey shiftkey deleted the release branch June 14, 2016 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shiftkey @ryangribble @haacked @hnrkndrssn