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 1 commit
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(int 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 int Id { get; protected set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to use long data type, since github could well have more than "max int" pull requests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. You did suggest long. I checked other class like issue, comment, they all using int type id. used int for consistency issue. As you mentioned we have more pullrequest than int can support already, I am going to change it to long again.

BTW, one of the check Travis CI build failed. I tried to read the log, but did not found any intuitive information. Do you mind to share me some tips on looking at that log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing is when I run the integration test, there is an authorizationException : bad credentials been thrown. Does that mean I need to change certain credentials to run the tests or I did not set some configurations right?

Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of running the integration tests, you need to configure your development environment (and is strongly recomended you setup a separate github account to use for the integration tests). More info is documented here in the contributors guide

Copy link
Contributor

Choose a reason for hiding this comment

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

And in terms of the TravisCI build sometimes we get an unrelated problem where it blows up in TravisCI and is not related to your changes. We have to kick off another build (or the next commit pushed will obviously trigger a new build as well). I see your latest commit, the builds have all passed 👍


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