use go-querystring for constructing URLs

willnorris · willnorris · commit 994f6f8405f0 · 2013-09-10T13:42:41.000-07:00
this only updates a few user and repo functions just to establish the calling pattern. closes #56 (which also includes a little more background)
diff --git a/github/github.go b/github/github.go
@@ -13,9 +13,12 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/url"
+	"reflect"
 	"strconv"
 	"strings"
 	"time"
+
+	"github.com/google/go-querystring/query"
 )
 
 const (
@@ -65,10 +68,32 @@ type Client struct {
 // support pagination.
 type ListOptions struct {
 	// For paginated result sets, page of results to retrieve.
-	Page int
+	Page int `url:"page,omitempty"`
 
 	// For paginated result sets, the number of results to include per page.
-	PerPage int
+	PerPage int `url:"per_page,omitempty"`
+}
+
+// addOptions adds the parameters in opt as URL query parameters to s.  opt
+// must be a struct whose fields may contain "url" tags.
+func addOptions(s string, opt interface{}) (string, error) {
+	v := reflect.ValueOf(opt)
+	if v.Kind() == reflect.Ptr && v.IsNil() {
+		return s, nil
+	}
+
+	u, err := url.Parse(s)
+	if err != nil {
+		return s, err
+	}
+
+	qs, err := query.Values(opt)
+	if err != nil {
+		return s, err
+	}
+
+	u.RawQuery = qs.Encode()
+	return u.String(), nil
 }
 
 // NewClient returns a new GitHub API client.  If a nil httpClient is
diff --git a/github/github_test.go b/github/github_test.go
@@ -57,10 +57,14 @@ func testMethod(t *testing.T, r *http.Request, want string) {
 type values map[string]string
 
 func testFormValues(t *testing.T, r *http.Request, values values) {
-	for key, want := range values {
-		if v := r.FormValue(key); v != want {
-			t.Errorf("Request parameter %v = %v, want %v", key, v, want)
-		}
+	want := url.Values{}
+	for k, v := range values {
+		want.Add(k, v)
+	}
+
+	r.ParseForm()
+	if !reflect.DeepEqual(want, r.Form) {
+		t.Errorf("Request parameters = %v, want %v", r.Form, want)
 	}
 }
 
diff --git a/github/repos.go b/github/repos.go
@@ -5,11 +5,7 @@
 
 package github
 
-import (
-	"fmt"
-	"net/url"
-	"strconv"
-)
+import "fmt"
 
 // RepositoriesService handles communication with the repository related
 // methods of the GitHub API.
@@ -59,18 +55,17 @@ func (r Repository) String() string {
 type RepositoryListOptions struct {
 	// Type of repositories to list.  Possible values are: all, owner, public,
 	// private, member.  Default is "all".
-	Type string
+	Type string `url:"type,omitempty"`
 
 	// How to sort the repository list.  Possible values are: created, updated,
 	// pushed, full_name.  Default is "full_name".
-	Sort string
+	Sort string `url:"sort,omitempty"`
 
 	// Direction in which to sort repositories.  Possible values are: asc, desc.
 	// Default is "asc" when sort is "full_name", otherwise default is "desc".
-	Direction string
+	Direction string `url:"direction,omitempty"`
 
-	// For paginated result sets, page of results to retrieve.
-	Page int
+	ListOptions
 }
 
 // List the repositories for a user.  Passing the empty string will list
@@ -84,14 +79,9 @@ func (s *RepositoriesService) List(user string, opt *RepositoryListOptions) ([]R
 	} else {
 		u = "user/repos"
 	}
-	if opt != nil {
-		params := url.Values{
-			"type":      []string{opt.Type},
-			"sort":      []string{opt.Sort},
-			"direction": []string{opt.Direction},
-			"page":      []string{strconv.Itoa(opt.Page)},
-		}
-		u += "?" + params.Encode()
+	u, err := addOptions(u, opt)
+	if err != nil {
+		return nil, nil, err
 	}
 
 	req, err := s.client.NewRequest("GET", u, nil)
@@ -109,23 +99,19 @@ func (s *RepositoriesService) List(user string, opt *RepositoryListOptions) ([]R
 type RepositoryListByOrgOptions struct {
 	// Type of repositories to list.  Possible values are: all, public, private,
 	// forks, sources, member.  Default is "all".
-	Type string
+	Type string `url:"type,omitempty"`
 
-	// For paginated result sets, page of results to retrieve.
-	Page int
+	ListOptions
 }
 
 // ListByOrg lists the repositories for an organization.
 //
 // GitHub API docs: https://developer.github.com/v3/repos/#list-organization-repositories
 func (s *RepositoriesService) ListByOrg(org string, opt *RepositoryListByOrgOptions) ([]Repository, *Response, error) {
 	u := fmt.Sprintf("orgs/%v/repos", org)
-	if opt != nil {
-		params := url.Values{
-			"type": []string{opt.Type},
-			"page": []string{strconv.Itoa(opt.Page)},
-		}
-		u += "?" + params.Encode()
+	u, err := addOptions(u, opt)
+	if err != nil {
+		return nil, nil, err
 	}
 
 	req, err := s.client.NewRequest("GET", u, nil)
@@ -142,19 +128,18 @@ func (s *RepositoriesService) ListByOrg(org string, opt *RepositoryListByOrgOpti
 // RepositoriesService.ListAll method.
 type RepositoryListAllOptions struct {
 	// ID of the last repository seen
-	Since int
+	Since int `url:"since,omitempty"`
+
+	ListOptions
 }
 
 // ListAll lists all GitHub repositories in the order that they were created.
 //
-// GitHub API docs: https://developer.github.com/v3/repos/#list-all-repositories
+// GitHub API docs: https://developer.github.com/v3/repos/#list-all-public-repositories
 func (s *RepositoriesService) ListAll(opt *RepositoryListAllOptions) ([]Repository, *Response, error) {
-	u := "repositories"
-	if opt != nil {
-		params := url.Values{
-			"since": []string{strconv.Itoa(opt.Since)},
-		}
-		u += "?" + params.Encode()
+	u, err := addOptions("repositories", opt)
+	if err != nil {
+		return nil, nil, err
 	}
 
 	req, err := s.client.NewRequest("GET", u, nil)
diff --git a/github/repos_test.go b/github/repos_test.go
@@ -49,7 +49,7 @@ func TestRepositoriesService_List_specifiedUser(t *testing.T) {
 		fmt.Fprint(w, `[{"id":1}]`)
 	})
 
-	opt := &RepositoryListOptions{"owner", "created", "asc", 2}
+	opt := &RepositoryListOptions{"owner", "created", "asc", ListOptions{Page: 2}}
 	repos, _, err := client.Repositories.List("u", opt)
 	if err != nil {
 		t.Errorf("Repositories.List returned error: %v", err)
@@ -79,7 +79,7 @@ func TestRepositoriesService_ListByOrg(t *testing.T) {
 		fmt.Fprint(w, `[{"id":1}]`)
 	})
 
-	opt := &RepositoryListByOrgOptions{"forks", 2}
+	opt := &RepositoryListByOrgOptions{"forks", ListOptions{Page: 2}}
 	repos, _, err := client.Repositories.ListByOrg("o", opt)
 	if err != nil {
 		t.Errorf("Repositories.ListByOrg returned error: %v", err)
@@ -102,11 +102,15 @@ func TestRepositoriesService_ListAll(t *testing.T) {
 
 	mux.HandleFunc("/repositories", func(w http.ResponseWriter, r *http.Request) {
 		testMethod(t, r, "GET")
-		testFormValues(t, r, values{"since": "1"})
+		testFormValues(t, r, values{
+			"since":    "1",
+			"page":     "2",
+			"per_page": "3",
+		})
 		fmt.Fprint(w, `[{"id":1}]`)
 	})
 
-	opt := &RepositoryListAllOptions{1}
+	opt := &RepositoryListAllOptions{1, ListOptions{2, 3}}
 	repos, _, err := client.Repositories.ListAll(opt)
 	if err != nil {
 		t.Errorf("Repositories.ListAll returned error: %v", err)
diff --git a/github/users.go b/github/users.go
@@ -7,8 +7,7 @@ package github
 
 import (
 	"fmt"
-	"net/url"
-	"strconv"
+
 	"time"
 )
 
@@ -83,19 +82,16 @@ func (s *UsersService) Edit(user *User) (*User, *Response, error) {
 // method.
 type UserListOptions struct {
 	// ID of the last user seen
-	Since int
+	Since int `url:"since,omitempty"`
 }
 
 // ListAll lists all GitHub users.
 //
 // GitHub API docs: https://developer.github.com/v3/users/#get-all-users
 func (s *UsersService) ListAll(opt *UserListOptions) ([]User, *Response, error) {
-	u := "users"
-	if opt != nil {
-		params := url.Values{
-			"since": []string{strconv.Itoa(opt.Since)},
-		}
-		u += "?" + params.Encode()
+	u, err := addOptions("users", opt)
+	if err != nil {
+		return nil, nil, err
 	}
 
 	req, err := s.client.NewRequest("GET", u, nil)
