Implemented HooksService (w/tests) #23

stianeikeland · 2013-07-31T18:59:24Z

Added support for manipulating hooks via the hooks api

Added support for manipulating webhooks for repositories

willnorris · 2013-07-31T19:15:24Z

Stian, thanks for the pull request. A couple of notes:

Google requires contributors to sign a CLA. I don't see one on file for you, but feel free to contact me if it's under a different name or something.
I've been trying to structure the library the same way that GitHub structures their API documentation. So the Hook methods would be part of the existing RepositoriesService rather than a new service.

stianeikeland · 2013-07-31T19:29:36Z

Google requires contributors to sign a CLA. I don't see one on file for you, but feel free to contact me if it's under a different name or something.

Submitted CLA: "Thank you. Your CLA submission will be processed shortly."

the Hook methods would be part of the existing RepositoriesService rather than a new service.

Ah, of course, my bad. I'll refactor and push the changes.

wlynch · 2013-07-31T19:40:16Z

Oops. Looks like you preempted me on these changes. No worries though, I'll go ahead and comment on differences we have.

wlynch · 2013-07-31T19:41:31Z

github/hooks.go

+	Name      string            `json:"name"`
+	Events    []string          `json:"events,omitempty"`
+	Active    bool              `json:"active"`
+	Config    map[string]string `json:"config,omitempty"`


This should be map[string]interface{}. Values can also be bools, ints, etc.

Looking at https://api.github.com/hooks I do see some boolean values (as well as "password" types, WTF?). I don't actually see any ints, but you're right, this should probably be map[string]interface{}

wlynch · 2013-07-31T19:58:40Z

One idea I toyed around with is the idea of separating the Hook struct into parts: Hook and an anonymous struct HookOptions. The motivation behind this was to separate the fields that the user can edit vs the fields that get returned back to the user. For example, the user can't edit the repo ID or Github URL, and that's not entirely clear from a simple hook struct. Making an anonymous substruct does have the benefit of being able to reference all fields as if they were directly part of the hook struct.

You can find an example of this on my fork. Let me know what you think about this.

Hook api calls belong to the repository service

willnorris · 2013-07-31T20:17:18Z

Couldn't the same thing be achieved by documenting the struct fields, either individually or breaking them into groups? That's actually what I did for Repository here, which works pretty well with the generated docs.

I haven't done a good job elsewhere in the API of identifying which fields are mutable 😦. That's actually one of the reasons I made sure to include links back to the GitHub API docs... for those things that aren't clear in the library, you can always refer to the GitHub docs. Of course, ideally you shouldn't have to... it should be clear in the Go source. But I think I'd rather try to accomplish that with docs rather than nested structs if we can. It keeps the overall surface area of the library a little more manageable (and it's already getting pretty big as it is 😕)

wlynch · 2013-07-31T20:18:20Z

github/repos.go

+	Events    []string               `json:"events,omitempty"`
+	Active    bool                   `json:"active"`
+	Config    map[string]interface{} `json:"config,omitempty"`
+	Id        int                    `json:"id,omitempty"`


Small nit here: general convention is to make acronyms all upper or lower case, never mixed. For example, "URL" or "url" is good, but not "Url". This rule also applies to ID.

Fixed, thank you :)

wlynch · 2013-07-31T20:35:47Z

I suppose that works. I can see where it makes more sense with other structs to keep everything together as one (like keeping Name as part of Repository).

wlynch · 2013-07-31T20:39:43Z

ListHooks looks like it suffers from the same problem as Issue #22. Do we want to do something similar to 1011d06?

stianeikeland · 2013-07-31T20:44:08Z

github/repos.go

+	UpdatedAt *time.Time             `json:"updated_at,omitempty"`
+	Name      string                 `json:"name"`
+	Events    []string               `json:"events,omitempty"`
+	Active    bool                   `json:"active"`


I noticed while testing this that the json serializer skip bool fields that are false if omitempty is enabled.
This meant that it was not possible to deactivate a webhook as "active":false was dropped from the json, only possible to activate one ("active":true worked). I removed omitempty from the bool field here because of this.

This has the side effect that all Hooks are per default Active = false. The default from github would normally be active: true.
Any idea how to fix this if needed? (it isn't possible to set default values for a struct in Go afaik)

yep, this problem is pretty well documented in #19. The solution will be to change all of these fields to be pointers, I just haven't sat down to do that yet. I'll probably do it sometime this week.

so for now, leave the omitempty in there.

willnorris · 2013-07-31T20:44:14Z

yep, should definitely do that. Since ListHooks() doesn't have any other options, we can just pass a ListOptions object rather than creating a new ListHooksOptions.

wlynch · 2013-07-31T21:14:03Z

As a general discussion: Are we going to continue to keep everything under one large repos.go file are are we ever going to split up each API into its own file (hooks.go, statuses.go, forks.go, etc.)?

willnorris · 2013-07-31T21:16:50Z

merged as d79a5b0

willnorris · 2013-07-31T21:19:37Z

As a general discussion: Are we going to continue to keep everything under one large repos.go file are are we ever going to split up each API into its own file (hooks.go, statuses.go, forks.go, etc.)?

thus far, I've kept everything in a single file per service... I haven't felt a strong need to separate them out just yet. I did leave events.go as its own file, even though it's really documented as part of the activity service, mainly because that file is going to get pretty big once we build out all the different event types.

willnorris · 2013-07-31T21:21:11Z

(just closing the PR since the code is now merged in... happy to continue discussing code organization)

wlynch · 2013-07-31T21:39:31Z

I was thinking about adopting some sort of naming scheme that would easily allow us to separate and distinguish source files and the APIs they provide. Instead of looking through repos.go (which after adding all 12 APIs, might be very lengthy) for the ListHook function, why not separate the hook API into something like repos_hooks.go. This lets us sort APIs into separate files while still keeping everything in a single package and making a clear relation to the service they are a part of. It also is a close mapping to the overall layout of the Github API (i.e. http://developer.github.com/v3/repos/ -> repos.go and http://developer.github.com/v3/repos/hooks/ -> repos_hooks.go).

I think this is something we should definitely consider for a large services such as RepositoryService, and apply to smaller services as a matter of consistency.

willnorris · 2013-07-31T21:47:45Z

yeah, I like that naming pattern. let's go for it 🤘

The rebase did not catch the fact that the test file was modified. This applies the changes in google#23 to the new test introduced in google#22.

Implemented HooksService (w/tests)

7b4a347

Added support for manipulating webhooks for repositories

wlynch reviewed Jul 31, 2013
View reviewed changes

stianeikeland added 2 commits July 31, 2013 22:04

Moved Hook to Repositories

8c1c72b

Hook api calls belong to the repository service

Hook Config can be other datatypes, use interface

dc89b3b

wlynch reviewed Jul 31, 2013
View reviewed changes

Renamed Hook Id to ID (lint)

abb3a96

stianeikeland reviewed Jul 31, 2013
View reviewed changes

stianeikeland added 2 commits July 31, 2013 22:50

Set omitempty for Hook Active and Name field

2822852

Added pagination (ListOptions) for ListHooks

b0e820d

willnorris closed this Jul 31, 2013

willnorris mentioned this pull request Jul 31, 2013

reorganize code into separate files per API #24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented HooksService (w/tests) #23

Implemented HooksService (w/tests) #23

stianeikeland commented Jul 31, 2013

willnorris commented Jul 31, 2013

stianeikeland commented Jul 31, 2013

wlynch commented Jul 31, 2013

wlynch Jul 31, 2013

willnorris Jul 31, 2013

wlynch commented Jul 31, 2013

willnorris commented Jul 31, 2013

wlynch Jul 31, 2013

stianeikeland Jul 31, 2013

wlynch commented Jul 31, 2013

wlynch commented Jul 31, 2013

stianeikeland Jul 31, 2013

willnorris Jul 31, 2013

willnorris Jul 31, 2013

willnorris commented Jul 31, 2013

wlynch commented Jul 31, 2013

willnorris commented Jul 31, 2013

willnorris commented Jul 31, 2013

willnorris commented Jul 31, 2013

wlynch commented Jul 31, 2013

willnorris commented Jul 31, 2013

Implemented HooksService (w/tests) #23

Implemented HooksService (w/tests) #23

Conversation

stianeikeland commented Jul 31, 2013

willnorris commented Jul 31, 2013

stianeikeland commented Jul 31, 2013

wlynch commented Jul 31, 2013

wlynch Jul 31, 2013

Choose a reason for hiding this comment

willnorris Jul 31, 2013

Choose a reason for hiding this comment

wlynch commented Jul 31, 2013

willnorris commented Jul 31, 2013

wlynch Jul 31, 2013

Choose a reason for hiding this comment

stianeikeland Jul 31, 2013

Choose a reason for hiding this comment

wlynch commented Jul 31, 2013

wlynch commented Jul 31, 2013

stianeikeland Jul 31, 2013

Choose a reason for hiding this comment

willnorris Jul 31, 2013

Choose a reason for hiding this comment

willnorris Jul 31, 2013

Choose a reason for hiding this comment

willnorris commented Jul 31, 2013

wlynch commented Jul 31, 2013

willnorris commented Jul 31, 2013

willnorris commented Jul 31, 2013

willnorris commented Jul 31, 2013

wlynch commented Jul 31, 2013

willnorris commented Jul 31, 2013