Add Id field to PullRequest class #1537
Conversation
added the Id property for PullRequest model
| @@ -48,6 +49,11 @@ public PullRequest(Uri url, Uri htmlUrl, Uri diffUrl, Uri patchUrl, Uri issueUrl | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// The universal Id (different from the pull request number) this pull request. | |||
| /// </summary> | |||
| public int Id { get; protected set; } | |||
There was a problem hiding this comment.
It would be good to use long data type, since github could well have more than "max int" pull requests!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍
| @@ -48,6 +49,11 @@ public PullRequest(Uri url, Uri htmlUrl, Uri diffUrl, Uri patchUrl, Uri issueUrl | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// The universal Id (different from the pull request number) this pull request. | |||
There was a problem hiding this comment.
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 😀
make it more intuitive... a question, issue.Id and comment.Id are int type. do we have any concern on it?
forgot to check this change.
|
I am not familiar with git hub. somehow I cannot find the comments I received in my email. Once you are done with this one, can you assign another entrylevel task? |
| @@ -59,6 +59,7 @@ public PullRequestsClientTests() | |||
|
|
|||
| Assert.Equal(1, pullRequests.Count); | |||
| Assert.Equal(result.Title, pullRequests[0].Title); | |||
| Assert.NotNull(pullRequests[0].Id); | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Thanks for the catch. I am going to update that. I am assuming all internal ids are larger than 0.
long will never be null
If the comments were on some lines of code that have since been replaced, then github collapses it into a "outdated" section. You can click that to see the comments again:
That's awesome! All of our issues with a tag of "up for grabs" are meant to be easy enough for new contributors to tackle. You can choose anything that takes your fancy but if you really want a suggestion how about #1514 😀 When you find an issue you want to work on, Just comment on that issue that you want to take a look at it (this makes sure we dont have multiple people trying to fix the same thing!). Create a branch on your fork of the repo (I notice you used your master branch for this PR, instead you should create a branch and do the work there, so your master branch can just be a identical copy of this repo's master branch) push that branch up and raise a pull request. Put [WIP] ( meaning work in progress) in the PR title, and we can then work together as you make the change. It's totally fine to raise the [WIP] pull request early in the process so we can collaborate rather than waiting until you've done lots of coding! |
|
Many thanks @YunLi1988 for this enhancement, and congratulations on your first octokit.net contribution! |
|
PS: I noticed the commits on this PR are from a different email address than your github profile, so they arent linked together. You should add your other email address to your profile so the commits are associated with your github account! 👍 |
ryangribble
changed the title
nickfloyd
added
Type: Feature
added the Id property for PullRequest model