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. |
phadej
left a comment
There was a problem hiding this comment.
Looks good. Small copy&paste mistake inline.
If you mind to run stylish-haskell on files you touched, that would be great too.
Thanks.
GHC-7.8.4 travis error is something weird. I'll figure that out.
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
|
|
||
| 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.
| 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? |
| 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