fix the PullRequest object so it can parse#285
Conversation
| | PullRequestUnassigned | ||
| | PullRequestLabeled | ||
| | PullRequestUnlabeled | ||
| | PullRequestReviewRequested |
There was a problem hiding this comment.
added these because the API docs list them.
Not sure if it makes sense to make a catch-all OtherPullRequestEventType Text which might make the library a little more future-proof.
|
Not sure why the PR builder is unable to find |
| <*> o .: "html_url" | ||
| <*> o .: "updated_at" | ||
| <*> o .:? "body" | ||
| <*> o .: "assignees" |
There was a problem hiding this comment.
I'm 100% sure that API returns a list, and GitHub docs are wrong. You can have multiple assignees for PR/Issue.
There was a problem hiding this comment.
Check: https://api.github.com/repos/phadej/github/pulls/285, there are both keys "assignee" and "assignees"
|
What do you mean by using the real world response as a fixture? |
|
@adnelson e.g. the current version of result for that the PR in your fixture https://api.github.com/repos/baxterthehacker/public-repo/pulls/1 has Real-world: don't copy old fixtures from somewhere, grab new ones from the API. |
|
@phadej I think the problem was that I was confusing the |
|
Alright, so I added three more fixtures, one that I had of the pull request event, and the two that you had given for pull requests, making four test cases in total. I added some logic which allows either the |
| , pullRequestCommits :: !Count | ||
| , pullRequestMerged :: !Bool | ||
| , pullRequestMergeable :: !(Maybe Bool) | ||
| , pullRequestMergeableState :: !MergeableState |
There was a problem hiding this comment.
why this is removed? (and review comments?)
There was a problem hiding this comment.
ah sorry, that was due to my confusion between pull request / pull request event. I'll put it back in
|
|
||
| -- | Helper function, reads either the "assignee" OR "assigneed" OR | ||
| -- both from a JSON object. | ||
| getAssignees :: Object -> Parser (Vector SimpleUser) |
There was a problem hiding this comment.
I'd trust there's always assignees, if there and endpoint which doesn't return plural assignees?
There was a problem hiding this comment.
this example has an assignee key but no assignees keys.
There was a problem hiding this comment.
But its current variant https://api.github.com/repos/octocat/Hello-World/pulls does have assignees. GitHub documentation is outdated, because examples are manually written.
There was a problem hiding this comment.
Is there a downside to including both? If we include both then it will work either way. You yourself earlier posted an example which contained both keys (as does the one in that link). I'm fine removing it but this seems like the way to accommodate all configurations which are likely to appear.
There was a problem hiding this comment.
@phadej if it's a dealbreaker for getting this merged then I can remove the assignee part
|
@phadej any thoughts on this? |
|
@phadej just checking in here... |
|
@adnelson : New maintainer here. |
andreasabel
left a comment
andreasabel
left a comment
There was a problem hiding this comment.
Please resolve conflicts.
3 participants
I'm not positive this covers every variant of a pull request event, but prior to this change the
FromJSONinstance failed to decode the example json (which is where the JSON I included here comes from) in the API docs, and now it does.