Skip to content

fix the PullRequest object so it can parse #285

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.

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
314 changes: 314 additions & 0 deletions fixtures/pull-request-baxterthehacker.json

Large diffs are not rendered by default.

415 changes: 415 additions & 0 deletions fixtures/pull-request-event-github-example.json

Large diffs are not rendered by default.

422 changes: 422 additions & 0 deletions fixtures/pull-request-event-realworld.json

Large diffs are not rendered by default.

350 changes: 350 additions & 0 deletions fixtures/pull-request-realworld.json

Large diffs are not rendered by default.

10 changes: 8 additions & 2 deletions github.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ extra-source-files:
fixtures/list-teams.json,
fixtures/members-list.json,
fixtures/user-organizations.json,
fixtures/user.json
fixtures/user.json,
fixtures/pull-request-realworld.json,
fixtures/pull-request-baxterthehacker.json,
fixtures/pull-request-event-github-example.json
fixtures/pull-request-event-realworld.json,

flag aeson-compat
description: Whether to use aeson-compat or aeson-extra
Expand Down Expand Up @@ -185,9 +189,11 @@ test-suite github-test
vector,
unordered-containers,
file-embed,
aeson,
text,
bytestring,
hspec
if flag(aeson-compat)
build-depends: aeson-compat
else
build-depends: aeson-extra

38 changes: 36 additions & 2 deletions spec/GitHub/PullRequestsSpec.hs
Original file line number Diff line number Diff line change
@@ -1,16 +1,24 @@
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TemplateHaskell #-}
module GitHub.PullRequestsSpec where

import qualified GitHub

import Prelude ()
import Prelude.Compat

import Data.Aeson.Compat (eitherDecodeStrict)
import Data.Either.Compat (isRight)
import Data.Foldable (for_)
import Data.String (fromString)
import qualified Data.Vector as V
import System.Environment (lookupEnv)
import Test.Hspec (Spec, describe, it, pendingWith, shouldSatisfy)
import Data.FileEmbed (embedFile)
import Test.Hspec (Spec, describe, it, pendingWith, shouldSatisfy, shouldBe)
import GitHub.Data.Definitions (simpleUserLogin)
import GitHub.Data.Options (IssueState(..))
import GitHub.Data.PullRequests

fromRightS :: Show a => Either a b -> b
fromRightS (Right b) = b
Expand All @@ -28,8 +36,34 @@ spec = do
describe "pullRequestsForR" $ do
it "works" $ withAuth $ \auth -> for_ repos $ \(owner, repo) -> do
cs <- GitHub.executeRequest auth $
GitHub.pullRequestsForR owner repo opts GitHub.FetchAll
GitHub.pullRequestsForR owner repo opts GitHub.FetchAll
cs `shouldSatisfy` isRight
describe "PullRequest" $ do
it "can parse PR json, from a PR to the github repo" $ do
let pr = fromRightS $
eitherDecodeStrict $(embedFile "fixtures/pull-request-realworld.json")

pullRequestState pr `shouldBe` StateOpen
simpleUserLogin (pullRequestUser pr) `shouldBe` "adnelson"
let [user] = V.toList (pullRequestAssignees pr)
simpleUserLogin user `shouldBe` "phadej"
it "can parse PR json, from the 'baxterthehacker' account" $ do
let pr = fromRightS $
eitherDecodeStrict $(embedFile "fixtures/pull-request-baxterthehacker.json")
pullRequestState pr `shouldBe` StateClosed
simpleUserLogin (pullRequestUser pr) `shouldBe` "baxterthehacker"
describe "PullRequestEvent" $ do
it "can parse PR event json, example from github docs" $ do
let pre = fromRightS $
eitherDecodeStrict $(embedFile "fixtures/pull-request-event-github-example.json")
pullRequestEventAction pre `shouldBe` PullRequestOpened
simpleUserLogin (pullRequestSender pre) `shouldBe` "baxterthehacker"
it "can parse PR event json, example from a real-world github repo" $ do
let pre = fromRightS $
eitherDecodeStrict $(embedFile "fixtures/pull-request-event-realworld.json")
pullRequestEventAction pre `shouldBe` PullRequestOpened
simpleUserLogin (pullRequestSender pre) `shouldBe` "adnelson"

where
repos =
[ ("thoughtbot", "paperclip")
Expand Down
40 changes: 30 additions & 10 deletions src/GitHub/Data/PullRequests.hs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@ module GitHub.Data.PullRequests (

import GitHub.Data.Definitions
import GitHub.Data.Id (Id)
import GitHub.Data.Options (IssueState (..), MergeableState (..))
import GitHub.Data.Options (IssueState (..))
import GitHub.Data.Repos (Repo)
import GitHub.Data.Request (StatusMap)
import GitHub.Data.URL (URL)
import GitHub.Internal.Prelude
import Data.Vector as V (elem, snoc)
import Data.Aeson.Types (Parser)
import qualified Data.Text as T
import Prelude ()

data SimplePullRequest = SimplePullRequest
Expand All @@ -36,7 +39,7 @@ data SimplePullRequest = SimplePullRequest
, simplePullRequestHtmlUrl :: !URL
, simplePullRequestUpdatedAt :: !UTCTime
, simplePullRequestBody :: !(Maybe Text)
, simplePullRequestAssignees :: (Vector SimpleUser)
, simplePullRequestAssignees :: !(Vector SimpleUser)
, simplePullRequestIssueUrl :: !URL
, simplePullRequestDiffUrl :: !URL
, simplePullRequestUrl :: !URL
Expand All @@ -60,7 +63,7 @@ data PullRequest = PullRequest
, pullRequestHtmlUrl :: !URL
, pullRequestUpdatedAt :: !UTCTime
, pullRequestBody :: !(Maybe Text)
, pullRequestAssignees :: (Vector SimpleUser)
, pullRequestAssignees :: !(Vector SimpleUser)
, pullRequestIssueUrl :: !URL
, pullRequestDiffUrl :: !URL
, pullRequestUrl :: !URL
Expand All @@ -74,12 +77,10 @@ data PullRequest = PullRequest
, pullRequestComments :: !Count
, pullRequestDeletions :: !Count
, pullRequestAdditions :: !Count
, pullRequestReviewComments :: !Count
, pullRequestBase :: !PullRequestCommit
, pullRequestCommits :: !Count
, pullRequestMerged :: !Bool
, pullRequestMergeable :: !(Maybe Bool)
, pullRequestMergeableState :: !MergeableState
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is removed? (and review comments?)

Copy link
Author

Choose a reason for hiding this comment

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

ah sorry, that was due to my confusion between pull request / pull request event. I'll put it back in

}
deriving (Show, Data, Typeable, Eq, Ord, Generic)

Expand Down Expand Up @@ -160,6 +161,9 @@ data PullRequestEventType
| PullRequestUnassigned
| PullRequestLabeled
| PullRequestUnlabeled
| PullRequestReviewRequested
Copy link
Author

Choose a reason for hiding this comment

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

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.

| PullRequestReviewRequestRemoved
| PullRequestEdited
deriving (Show, Data, Typeable, Eq, Ord, Generic)

instance NFData PullRequestEventType where rnf = genericRnf
Expand All @@ -180,8 +184,22 @@ instance Binary PullRequestReference
-- JSON instances
-------------------------------------------------------------------------------

-- | Helper function, reads either the "assignee" OR "assigneed" OR
-- both from a JSON object.
getAssignees :: Object -> Parser (Vector SimpleUser)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd trust there's always assignees, if there and endpoint which doesn't return plural assignees?

Copy link
Author

Choose a reason for hiding this comment

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

this example has an assignee key but no assignees keys.

Copy link
Contributor

@phadej phadej Aug 2, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Author

@adnelson adnelson Aug 3, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@phadej if it's a dealbreaker for getting this merged then I can remove the assignee part

getAssignees o = do
assignees <- o .:? "assignees" .!= mempty
maybeAssignee <- o .:? "assignee"
pure $ case maybeAssignee of
Nothing -> assignees
Just assignee | assignee `V.elem` assignees -> assignees
| otherwise -> assignees `V.snoc` assignee

instance FromJSON SimplePullRequest where
parseJSON = withObject "SimplePullRequest" $ \o -> SimplePullRequest
parseJSON = withObject "SimplePullRequest" $ \o -> do
-- | Either key, or both, might be present, and might contain
-- redundant information. Take both keys and uniquify the list.
SimplePullRequest
<$> o .:? "closed_at"
<*> o .: "created_at"
<*> o .: "user"
Expand All @@ -191,7 +209,7 @@ instance FromJSON SimplePullRequest where
<*> o .: "html_url"
<*> o .: "updated_at"
<*> o .:? "body"
<*> o .: "assignees"
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 100% sure that API returns a list, and GitHub docs are wrong. You can have multiple assignees for PR/Issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check: https://api.github.com/repos/phadej/github/pulls/285, there are both keys "assignee" and "assignees"

<*> getAssignees o
<*> o .: "issue_url"
<*> o .: "diff_url"
<*> o .: "url"
Expand Down Expand Up @@ -231,7 +249,7 @@ instance FromJSON PullRequest where
<*> o .: "html_url"
<*> o .: "updated_at"
<*> o .:? "body"
<*> o .: "assignees"
<*> getAssignees o
<*> o .: "issue_url"
<*> o .: "diff_url"
<*> o .: "url"
Expand All @@ -245,12 +263,10 @@ instance FromJSON PullRequest where
<*> o .: "comments"
<*> o .: "deletions"
<*> o .: "additions"
<*> o .: "review_comments"
<*> o .: "base"
<*> o .: "commits"
<*> o .: "merged"
<*> o .:? "mergeable"
<*> o .: "mergeable_state"

instance FromJSON PullRequestLinks where
parseJSON = withObject "PullRequestLinks" $ \o -> PullRequestLinks
Expand Down Expand Up @@ -284,6 +300,10 @@ instance FromJSON PullRequestEventType where
parseJSON (String "unassigned") = pure PullRequestUnassigned
parseJSON (String "labeled") = pure PullRequestLabeled
parseJSON (String "unlabeled") = pure PullRequestUnlabeled
parseJSON (String "review_requested") = pure PullRequestReviewRequested
parseJSON (String "review_request_removed") = pure PullRequestReviewRequestRemoved
parseJSON (String "edited") = pure PullRequestEdited
parseJSON (String s) = fail $ "Unknown action type " <> T.unpack s
parseJSON v = typeMismatch "Could not build a PullRequestEventType" v

instance FromJSON PullRequestReference where
Expand Down