Add notifications endpoint #324
Conversation
|
Not sure what happend with fo r 7.8... |
|
@phadej Any chance you can take a look at this. I'd like to merge something to taffybar, that needs this change. |
src/GitHub/Data/Activities.hs
Outdated
| "state_change" -> pure StateChangeReason | ||
| "subscribed" -> pure SubscribedReason | ||
| "team_mention" -> pure TeamMentionReason | ||
| _ -> fail $ "Unknown EventType " ++ show t |
src/GitHub/Data/Activities.hs
Outdated
| @@ -5,8 +5,11 @@ | |||
| -- | |||
| module GitHub.Data.Activities where | |||
|
|
|||
| import GitHub.Data.Repos (Repo) | |||
| import GitHub.Data.Id (Id) | |||
| import GitHub.Data.Repos (RepoRef, Repo) | |||
There was a problem hiding this comment.
stylish-haskell formats imports for us.
src/GitHub/Data/Request.hs
Outdated
| @@ -117,7 +117,7 @@ instance NFData FetchCount where rnf = genericRnf | |||
| -- or aren't read-only. | |||
| data RW | |||
| = RO -- ^ /Read-only/, doesn't necessarily requires authentication | |||
| | RA -- ^ /Read autenticated/ | |||
| | RA -- ^ /Read authenticated/ | |||
|
Thanks @phadej. I've updated the pr. Are you planning to do a release any time soon? |
| @@ -95,7 +95,7 @@ instance Binary Notification | |||
|
|
|||
| instance FromJSON Notification where | |||
| parseJSON = withObject "Notification" $ \o -> Notification | |||
| <$> o .: "id" | |||
| <$> (mkId undefined . read <$> o .: "id") | |||
There was a problem hiding this comment.
sorry, not sure what you are asking.
Are you asking about the undefined. I don't think ti should matter since its simply an uneeded (because the type can be inferred in this case) proxy.
There was a problem hiding this comment.
So then Proxy works too, yes?
There was a problem hiding this comment.
Doesn't original
<$> o .: idwork? Why?
There was a problem hiding this comment.
@phadej For some reason, github uses a string type for the notification Id even though it uses a number type for all other ids. Not sure why they do that
There was a problem hiding this comment.
@TomMD ahh, didn't know about Proxy. Seems like it is used pretty widely in this code base. I can make that change.
There was a problem hiding this comment.
@phadej Are you saying <$> o .: id would work even though the value is a string, not an int?
|
@phadej ping? any chance thsi can be merged soon? |
|
@phadej ping. I think I fixed all of the issues you raised. Is there something else you wanted me to do? |
|
Rebased into #372 |
Fixes #323