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 Id field to PullRequest class #1537

Merged
merged 5 commits into from
Jan 14, 2017
Merged

Add Id field to PullRequest class #1537

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a2955d8
add Id for PullRequest.cs
Jan 12, 2017
dbe3ebc
use long for pull request id
Jan 12, 2017
a90b9d2
update comments for PullRequest.Id and Issue.Id
Jan 13, 2017
f46db33
plus the one for pullrequest
Jan 13, 2017
51d1f43
update the integration test for Id Property
Jan 13, 2017
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
1 change: 1 addition & 0 deletions Octokit.Tests.Integration/Clients/PullRequestsClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public async Task CanGetForRepository()

Assert.Equal(1, pullRequests.Count);
Assert.Equal(result.Title, pullRequests[0].Title);
Assert.NotNull(pullRequests[0].Id);
Copy link
Contributor

Choose a reason for hiding this comment

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

This int field can never be null so I'm thinking we will need to assert something else here (eg Assert.True(pullRequests[0].Id > 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. I am going to update that. I am assuming all internal ids are larger than 0.

}

[IntegrationTest]
Expand Down
8 changes: 7 additions & 1 deletion Octokit/Models/Response/PullRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ public PullRequest(int number)
Number = number;
}

public PullRequest(Uri url, Uri htmlUrl, Uri diffUrl, Uri patchUrl, Uri issueUrl, Uri statusesUrl, int number, ItemState state, string title, string body, DateTimeOffset createdAt, DateTimeOffset updatedAt, DateTimeOffset? closedAt, DateTimeOffset? mergedAt, GitReference head, GitReference @base, User user, User assignee, IReadOnlyList<User> assignees, bool? mergeable, User mergedBy, int comments, int commits, int additions, int deletions, int changedFiles, Milestone milestone, bool locked)
public PullRequest(long id, Uri url, Uri htmlUrl, Uri diffUrl, Uri patchUrl, Uri issueUrl, Uri statusesUrl, int number, ItemState state, string title, string body, DateTimeOffset createdAt, DateTimeOffset updatedAt, DateTimeOffset? closedAt, DateTimeOffset? mergedAt, GitReference head, GitReference @base, User user, User assignee, IReadOnlyList<User> assignees, bool? mergeable, User mergedBy, int comments, int commits, int additions, int deletions, int changedFiles, Milestone milestone, bool locked)
{
Id = id;
Url = url;
HtmlUrl = htmlUrl;
DiffUrl = diffUrl;
Expand Down Expand Up @@ -47,6 +48,11 @@ public PullRequest(Uri url, Uri htmlUrl, Uri diffUrl, Uri patchUrl, Uri issueUrl
Locked = locked;
}

/// <summary>
/// The universal Id (different from the pull request number) this pull request.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please reword this to "The internal Id for this pull request (not the pull request number)." ?

And could you edit the comment on Issue.Id to be similar? "The internal Id for this issue (not the issue number)."

Thanks 😀

/// </summary>
public long Id { get; protected set; }

/// <summary>
/// The URL for this pull request.
/// </summary>
Expand Down