Skip to content
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

Implement the Webhooks API #64

Merged
merged 1 commit into from
Jul 30, 2014
Merged

Conversation

nataren
Copy link
Collaborator

@nataren nataren commented Jul 27, 2014

Hello everyone. I am submitting this pull request for overall code review, I tried to follow the guidelines as much as possible but please feel free to comment on the code so that its quality improves, in particular for example:

  • I added the type synonym for RepoName and RepoOwner so that in the type signature you have a better idea what each string value means (I could even make it a tagged type but that might be overkill, what do you think?)
  • The records' field names are kind of long but it prevents the overriding of symbols named the same.

@nataren nataren mentioned this pull request Jul 28, 2014
data RepoWebhook = RepoWebhook {
repoWebhookUrl :: String
,repoWebhookTestUrl :: String
,repoWebhookId :: Int
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Integer.

@nataren
Copy link
Collaborator Author

nataren commented Jul 30, 2014

@maxpow4h: Here it goes, I made all the changes you mentioned.

Cheers
césar

@mxswd
Copy link
Contributor

mxswd commented Jul 30, 2014

@nataren Great work! Looks all good to me. I will move some of my code over to that API when I'm working on that next.

@nataren
Copy link
Collaborator Author

nataren commented Jul 30, 2014

Thanks a lot @maxpow4h 👍

@jwiegley I think this puppy is ready to roll in. Thanks.

jwiegley added a commit that referenced this pull request Jul 30, 2014
@jwiegley jwiegley merged commit 9309db0 into haskell-github:master Jul 30, 2014
@jwiegley
Copy link
Contributor

@nataren Thank you!

@jwiegley jwiegley mentioned this pull request Jul 30, 2014
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants