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

EventsService #4

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
101 changes: 101 additions & 0 deletions github/events.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// Copyright 2013 Google. All rights reserved.
//
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file or at
// https://developers.google.com/open-source/licenses/bsd

package github

import (
"encoding/json"
"fmt"
"net/url"
"strconv"
"time"
)

// EventsService provides access to the event related functions
// in the GitHub API.
//
// GitHub API docs: http://developer.github.com/v3/activity/events/
type EventsService struct {
client *Client
}

// Event represents a GitHub event.
type Event struct {
Type string `json:"type,omitempty"`
Public bool `json:"public"`
RawPayload json.RawMessage `json:"payload,omitempty"`
Repo *Repository `json:"repo,omitempty"`
Actor User `json:"actor,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you intentionally change Actor to no longer be a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because it appears to always be non-null in responses from GitHub's API. Would you prefer it to be a pointer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, go ahead and make it a pointer for consistency.

Org *Organization `json:"org,omitempty"`
CreatedAt *time.Time `json:"created_at,omitempty"`
ID string `json:"id,omitempty"`
}

// Payload returns the parsed event payload. For recognized event types
// (PushEvent), a value of the corresponding struct type will be returned.
func (e *Event) Payload() (payload interface{}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the best technique I could think of for dealing with the polymorphic payload value. I wanted it to support future event types by falling back to parsing to interface{}, and I wanted to support direct unmarshaling into an Event through the standard encoding/json library (instead of requiring the use of, e.g., a ParseEvent func, which could set an interface-typed Payload field).

(For comparison, the Google+ API client punts on this by just storing HTML; see https://code.google.com/p/google-api-go-client/source/browse/plus/v1/plus-gen.go#238.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(that's not actually why Google+ returns HTML in the content field)

Another approach would be to implement the json.Marshaler interface, and do this type-specific unmarshalling inside of a custom UnmarshalJSON function. That should allow for:

type Event struct {
  ...
  Payload interface{} `json:"payload,omit_empty"`
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this would let us unmarshal to different types dependent on the Type field of the parent struct.

On Jun 15, 2013, at 14:27, Will Norris notifications@github.com wrote:

In github/events.go:

+// Event represents a GitHub event.
+type Event struct {

  • Type string json:"type,omitempty"
  • Public bool json:"public"
  • RawPayload json.RawMessage json:"payload,omitempty"
  • Repo *Repository json:"repo,omitempty"
  • Actor User json:"actor,omitempty"
  • Org *Organization json:"org,omitempty"
  • CreatedAt *time.Time json:"created_at,omitempty"
  • ID string json:"id,omitempty"
    +}

+// Payload returns the parsed event payload. For recognized event types
+// (PushEvent), a value of the corresponding struct type will be returned.
+func (e *Event) Payload() (payload interface{}) {
(that's not actually why Google+ returns HTML in the content field)

Another approach would be to implement the json.Marshaler interface, and do this type-specific unmarshalling inside of a custom UnmarshalJSON function. That should allow for:

type Event struct {
...
Payload interface{} json:"payload,omit_empty"
}

Reply to this email directly or view it on GitHub.

Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't that the whole point of exposing the Marshaler interface... to allow structs to handle the marshalling however it wants? I'm pretty sure this should be possible, but it would likely require that the custom UnmarshalJSON be responsible for unmarhsalling the entire object, not just these one or two fields. That in and of itself may make it not worth dealing with, in which case the Payload() func you've got is a good alternative approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, I thought you had meant making the type of the Payload field (not the Event struct) implement Unmarshaler. I must've missed the code snippet when I replied on my phone.

switch e.Type {
case "PushEvent":
payload = &PushEvent{}
}
if err := json.Unmarshal(e.RawPayload, &payload); err != nil {
panic(err.Error())
}
return payload
}

// PushEvent represents a git push to a GitHub repository.
//
// GitHub API docs: http://developer.github.com/v3/activity/events/types/#pushevent
type PushEvent struct {
PushID int `json:"push_id,omitempty"`
Head string `json:"head,omitempty"`
Ref string `json:"ref,omitempty"`
Size int `json:"ref,omitempty"`
Commits []PushCommit `json:"commits,omitempty"`
}

// PushCommit represents a git commit in a GitHub PushEvent.
type PushCommit struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the representation of a commit in a PushEvent differs from that of a commit obtained via the Git data API (http://developer.github.com/v3/git/commits/), so we can't reuse the same type (and thus I named it "PushCommit" not "Commit").

Copy link
Collaborator

Choose a reason for hiding this comment

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

They're really close... I wonder if it's close enough to make the same type work for both, or will that be getting too messy?

Copy link
Contributor

Choose a reason for hiding this comment

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

This question will come up in all APIs I think - (it's the same with Tree and many more I believe), it comes down to the question discussed elsewhere "one type with nulls vs. many specific types". I think @willnorris voted for the one-type approach + good docs... I'd personally like the "many types" one, but we'd best agree on some
naming scheme for those then hm...

Just sayin' because I'll PullReq a few things soon (Trees API being ready for example), and I'm wondering which style to use.

(ah found the discussion #3 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll go with what Will wants and change these per his suggestions.

On Jun 15, 2013, at 15:30, Konrad Malawski notifications@github.com wrote:

In github/events.go:

  • return payload
    +}

+// PushEvent represents a git push to a GitHub repository.
+//
+// GitHub API docs: http://developer.github.com/v3/activity/events/types/#pushevent
+type PushEvent struct {

  • PushID int json:"push_id,omitempty"
  • Head string json:"head,omitempty"
  • Ref string json:"ref,omitempty"
  • Size int json:"ref,omitempty"
  • Commits []PushCommit json:"commits,omitempty"
    +}

+// PushCommit represents a git commit in a GitHub PushEvent.
+type PushCommit struct {
This question will come up in all APIs I think - (it's the same with Tree and many more I believe), it comes down to the question discussed elsewhere "one type with nulls vs. many specific types". I think @willnorris foted for the one-type approach + good docs... I'd personally like the "many types" one, but we'd best agree on some
naming scheme for those then hm...

Just sayin' because I'll PullReq a few things soon (Trees pretty much ready for example), and I'm wondering which style to use.


Reply to this email directly or view it on GitHub.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In order for PushCommitAuthor to be it's own type (per my above comment), it seems that PushCommit will need to be its own type as well... so again, let's go with your original design. I think I do still want to continue using a single type for simple cases where the API is just returning a partial representation of the same resource (as discussed on #3). But for cases like PushCommitAuthor where there really is a semantic distinction between the types, you're both right that separate types are warranted. Does that seem like a reasonable approach?

@ktoso: I haven't looked at the Trees API close enough yet to know which side it falls on. If you're unsure based on my comment above, let's discuss.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe I got the style we're aiming at, will have to change a slight detail in the commit and it should fit in :) Thanks for the the heads up 👍

SHA string `json:"sha,omitempty"`
Message string `json:"message,omitempty"`
Author PushCommitAuthor `json:"author,omitempty"`
URL string `json:"url,omitempty"`
Distinct bool `json:"distinct"`
}

// PushCommitAuthor represents the author of a git commit in a GitHub
// PushEvent.
type PushCommitAuthor struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, the representation of an author in a PushEvent's Commit is different from that of an author obtained via the Git data API (http://developer.github.com/v3/git/commits/), so this type needs to be specific to a PushCommit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is date the only difference? That might not be so bad.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this closer, you're absolutely right that commit author should be a separate type. This does not necessarily represent a GitHub user, so using the existing User type is not accurate. Plus, sticking the date in to User doesn't make any sense. So go with what you have here and just ignore my previous comment :)

Name string `json:"name,omitempty"`
Email string `json:"email,omitempty"`
}

// ListPerformedByUser lists the events performed by a user. If publicOnly is
// true, only public events will be returned.
//
// GitHub API docs: http://developer.github.com/v3/activity/events/#list-events-performed-by-a-user
func (s *EventsService) ListPerformedByUser(user string, publicOnly bool, opt *ListOptions) ([]Event, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

currently, the orgs service uses two different methods for public versus non-public collections (e.g. ListMembers() vs ListPublicMembers()). I'm somewhat ambivalent, so could go with either style, so long as it's consistent.

url_ := fmt.Sprintf("users/%v/events", user)
if publicOnly {
url_ += "/public"
Copy link
Collaborator

Choose a reason for hiding this comment

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

even though it's a little more verbose, I think the following style is a little clearer:

var url_ string
if publicOnly {
  url_ = fmt.Sprintf("users/%v/events/public", user)
} else {
  url_ = fmt.Sprintf("users/%v/events", user)
}

}
if opt != nil {
params := url.Values{
"page": []string{strconv.Itoa(opt.Page)},
}
url_ += "?" + params.Encode()
}

req, err := s.client.NewRequest("GET", url_, nil)
if err != nil {
return nil, err
}

events := new([]Event)
_, err = s.client.Do(req, events)
return *events, err
}
88 changes: 88 additions & 0 deletions github/events_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// Copyright 2013 Google. All rights reserved.
//
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file or at
// https://developers.google.com/open-source/licenses/bsd

package github

import (
"encoding/json"
"fmt"
"net/http"
"reflect"
"testing"
)

func TestEventsService_ListPerformedByUser_all(t *testing.T) {
setup()
defer teardown()

mux.HandleFunc("/users/u/events", func(w http.ResponseWriter, r *http.Request) {
if m := "GET"; m != r.Method {
t.Errorf("Request method = %v, want %v", r.Method, m)
}
fmt.Fprint(w, `[{"id":"1"},{"id":"2"}]`)
})

events, err := client.Events.ListPerformedByUser("u", false, nil)
if err != nil {
t.Errorf("Events.ListPerformedByUser returned error: %v", err)
}

want := []Event{Event{ID: "1"}, Event{ID: "2"}}
if !reflect.DeepEqual(events, want) {
t.Errorf("Events.ListPerformedByUser returned %+v, want %+v", events, want)
}
}

func TestEventsService_ListPerformedByUser_publicOnly(t *testing.T) {
setup()
defer teardown()

mux.HandleFunc("/users/u/events/public", func(w http.ResponseWriter, r *http.Request) {
if m := "GET"; m != r.Method {
t.Errorf("Request method = %v, want %v", r.Method, m)
}
fmt.Fprint(w, `[{"id":"1"},{"id":"2"}]`)
})

events, err := client.Events.ListPerformedByUser("u", true, nil)
if err != nil {
t.Errorf("Events.ListPerformedByUser returned error: %v", err)
}

want := []Event{Event{ID: "1"}, Event{ID: "2"}}
if !reflect.DeepEqual(events, want) {
t.Errorf("Events.ListPerformedByUser returned %+v, want %+v", events, want)
}
}

func TestEvent_Payload_typed(t *testing.T) {
raw := []byte(`{"type": "PushEvent","payload":{"push_id": 1}}`)
var event *Event
if err := json.Unmarshal(raw, &event); err != nil {
t.Fatalf("Unmarshal Event returned error: %v", err)
}

want := &PushEvent{PushID: 1}
if !reflect.DeepEqual(event.Payload(), want) {
t.Errorf("Event Payload returned %+v, want %+v", event.Payload(), want)
}
}

// TestEvent_Payload_untyped checks that unrecognized events are parsed to an
// interface{} value (instead of being discarded or throwing an error), for
// forward compatibility with new event types.
func TestEvent_Payload_untyped(t *testing.T) {
raw := []byte(`{"type": "UnrecognizedEvent","payload":{"field": "val"}}`)
var event *Event
if err := json.Unmarshal(raw, &event); err != nil {
t.Fatalf("Unmarshal Event returned error: %v", err)
}

want := map[string]interface{}{"field": "val"}
if !reflect.DeepEqual(event.Payload(), want) {
t.Errorf("Event Payload returned %+v, want %+v", event.Payload(), want)
}
}
2 changes: 2 additions & 0 deletions github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ type Client struct {

// Services used for talking to different parts of the API

Events *EventsService
Organizations *OrganizationsService
Repositories *RepositoriesService
Users *UsersService
Expand All @@ -98,6 +99,7 @@ func NewClient(httpClient *http.Client) *Client {
baseURL, _ := url.Parse(defaultBaseURL)

c := &Client{client: httpClient, BaseURL: baseURL, UserAgent: userAgent}
c.Events = &EventsService{client: c}
c.Organizations = &OrganizationsService{client: c}
c.Repositories = &RepositoriesService{client: c}
c.Users = &UsersService{client: c}
Expand Down