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

Add SHA and make message optional for pull request merge #805

Merged
merged 2 commits into from
May 19, 2015
Merged

Add SHA and make message optional for pull request merge #805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8cef69
Add SHA and make message optional
hnrkndrssn May 18, 2015
6cdb1cf
Add integration tests
hnrkndrssn May 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 45 additions & 2 deletions Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Linq;
using System.Threading.Tasks;
using Octokit;
using Octokit.Tests.Helpers;
using Octokit.Tests.Integration;
using Xunit;

Expand Down Expand Up @@ -193,12 +194,54 @@ public async Task CanBeMerged()
var newPullRequest = new NewPullRequest("a pull request", branchName, "master");
var pullRequest = await _fixture.Create(Helper.UserName, _repository.Name, newPullRequest);

var merge = new MergePullRequest("thing the thing");
var merge = new MergePullRequest { Message = "thing the thing" };
var result = await _fixture.Merge(Helper.UserName, _repository.Name, pullRequest.Number, merge);

Assert.True(result.Merged);
}

[IntegrationTest]
public async Task CanBeMergedWithNoOptionalInput()
{
await CreateTheWorld();

var newPullRequest = new NewPullRequest("a pull request", branchName, "master");
var pullRequest = await _fixture.Create(Helper.UserName, _repository.Name, newPullRequest);

var merge = new MergePullRequest();
var result = await _fixture.Merge(Helper.UserName, _repository.Name, pullRequest.Number, merge);

Assert.True(result.Merged);
}
[IntegrationTest]
public async Task CanBeMergedWithShaSpecified()
{
await CreateTheWorld();

var newPullRequest = new NewPullRequest("a pull request", branchName, "master");
var pullRequest = await _fixture.Create(Helper.UserName, _repository.Name, newPullRequest);

var merge = new MergePullRequest { Message = "thing the thing", Sha = pullRequest.Head.Sha };
var result = await _fixture.Merge(Helper.UserName, _repository.Name, pullRequest.Number, merge);

Assert.True(result.Merged);
}

[IntegrationTest]
public async Task CannotBeMerged()
{
await CreateTheWorld();
var fakeSha = new string('f', 40);

var newPullRequest = new NewPullRequest("a pull request", branchName, "master");
var pullRequest = await _fixture.Create(Helper.UserName, _repository.Name, newPullRequest);

var merge = new MergePullRequest { Sha = fakeSha };
var ex = await AssertEx.Throws<ApiException>(async () => await _fixture.Merge(Helper.UserName, _repository.Name, pullRequest.Number, merge));

Assert.True(ex.ApiError.Message.StartsWith("Head branch was modified"));
}

[IntegrationTest]
public async Task UpdatesMaster()
{
Expand All @@ -207,7 +250,7 @@ public async Task UpdatesMaster()
var newPullRequest = new NewPullRequest("a pull request", branchName, "master");
var pullRequest = await _fixture.Create(Helper.UserName, _repository.Name, newPullRequest);

var merge = new MergePullRequest("thing the thing");
var merge = new MergePullRequest { Message = "thing the thing" };
var result = await _fixture.Merge(Helper.UserName, _repository.Name, pullRequest.Number, merge);

var master = await _client.GitDatabase.Reference.Get(Helper.UserName, _repository.Name, "heads/master");
Expand Down
8 changes: 4 additions & 4 deletions Octokit.Tests/Clients/PullRequestsClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@ await AssertEx.Throws<ArgumentNullException>(() =>
public class TheMergeMethod
{
[Fact]
public void PutsToCorrectUrl()
public void PutsToCorrectUrl()
{
var mergePullRequest = new MergePullRequest("fake commit message");
var mergePullRequest = new MergePullRequest { Message = "fake commit message" };
var connection = Substitute.For<IApiConnection>();
var client = new PullRequestsClient(connection);

Expand All @@ -157,9 +157,9 @@ public async Task EnsuresArgumentsNotNull()
var client = new PullRequestsClient(connection);

await AssertEx.Throws<ArgumentNullException>(() =>
client.Merge(null, "name", 42, new MergePullRequest("message")));
client.Merge(null, "name", 42, new MergePullRequest { Message = "message" }));
await AssertEx.Throws<ArgumentNullException>(() =>
client.Merge("owner", null, 42, new MergePullRequest("message")));
client.Merge("owner", null, 42, new MergePullRequest { Message = "message" }));
await AssertEx.Throws<ArgumentNullException>(() =>
client.Merge("owner", "name", 42, null));
}
Expand Down
6 changes: 3 additions & 3 deletions Octokit.Tests/Reactive/ObservablePullRequestsClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ public class TheMergeMethod
[Fact]
public void MergesPullRequest()
{
var mergePullRequest = new MergePullRequest("fake commit message");
var mergePullRequest = new MergePullRequest { Message = "fake commit message" };
var gitHubClient = Substitute.For<IGitHubClient>();
var client = new ObservablePullRequestsClient(gitHubClient);

Expand All @@ -240,9 +240,9 @@ public async Task EnsuresArgumentsNotNull()
var client = new PullRequestsClient(connection);

await AssertEx.Throws<ArgumentNullException>(async () => await
client.Merge(null, "name", 42, new MergePullRequest("message")));
client.Merge(null, "name", 42, new MergePullRequest { Message = "message" }));
await AssertEx.Throws<ArgumentNullException>(async () => await
client.Merge("owner", null, 42, new MergePullRequest("message")));
client.Merge("owner", null, 42, new MergePullRequest { Message = "message" }));
await AssertEx.Throws<ArgumentNullException>(async () => await
client.Merge("owner", "name", 42, null));
}
Expand Down
16 changes: 7 additions & 9 deletions Octokit/Models/Request/MergePullRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,21 @@ namespace Octokit
[DebuggerDisplay("{DebuggerDisplay,nq}")]
public class MergePullRequest
{
public MergePullRequest(string message)
{
Ensure.ArgumentNotNull(message, "message");

Message = message;
}

/// <summary>
/// The message that will be used for the merge commit (optional)
/// </summary>
public string Message { get; private set; }
public string Message { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently fixing this, but according to the docs here: https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button

This is not optional and it should be CommitMessage. Did you happen to see something that suggests otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would've thought I did see something that suggested that the Message property was optional, but can't for the life of me remember (or find) where.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well the response is message but the request is commit_message. The docs are actually a bit unclear on this so I'll ask @pengwynn for clarification.


/// <summary>
/// The SHA that pull request head must match to allow merge (optional)
/// </summary>
public string Sha { get; set; }

internal string DebuggerDisplay
{
get
{
return String.Format(CultureInfo.InvariantCulture, "Message: {0}", Message);
return String.Format(CultureInfo.InvariantCulture, "Message: '{0}', Sha: '{1}'", Message, Sha);
}
}
}
Expand Down