fix: Improve pagination #96

osterbit · 2017-11-06T23:03:14Z

first pass:
- added a ListParamsFromRequest function to core/params.go
  - (also added default page size, default ordering to core/params.go)
- added core/params_test.go
- updated api/handlers' List functions/methods to use ListParamsFromRequest() instead of PageFromRequest() in
  - api/handlers/datasets
  - api/handlers/peers
  - api/handlers/queries

- first pass complete - Limit and Offset ListParams obtained by api/handlers' List functions/methods using `ListParamsFromRequest()` - `params_test.go` added, handles a few cases of invalid limits and offsets - partially complete - added support for accepting an 'orderBy' request query param just now but have not yet updated core/params_test and api/handlers functions to use this value yet

…y field

b5

lots of feedback that's almost all style oriented :)

b5 · 2017-11-06T23:12:24Z

api/handlers/datasets.go

+	// args := &core.ListParams{
+	// 	Limit:   lp.Limit,
+	// 	Offset:  lp.Offset,
+	// 	OrderBy: "created", //TODO: should there be a global default orderby?


re global default orderby, I'd say no, let's have implementations decide their default order, and have the "default" order passed in be empty string: "", which is the default value created by a new struct

b5 · 2017-11-06T23:14:27Z

api/handlers/datasets.go

+	// TODO: need to update util.WritePageResponse to take a
+	// core.ListParams rather than a util.Page struct
+	// for time being I added an empty util.Page struct
+	if err := util.WritePageResponse(w, res, r, util.Page{}); err != nil {


copying my slack comments here:

generally, I’d say that things going back to the api_util package should return types defined there. we should write a method on ListParams: func (l ListParams) AsPage() util.Page
then in practice we can use something like util.WritePageResponse(w, res, r, args.AsPage())

b5 · 2017-11-06T23:17:30Z

core/params.go

@@ -11,3 +26,34 @@ type ListParams struct {
 	Limit   int
 	Offset  int
 }
+
+// ListParamsFromRequest extracts and returns a ListParams struct
+// given a pointer to an http request *r


just for style purposes, I try to keep comments clear without referring to argument names (b/c often the code changes & people are bad at updating comments. I would write this as:

// ListParamsFromRequest extracts ListParams from an http.Request pointer

b5 · 2017-11-06T23:18:28Z

core/params.go

+func ListParamsFromRequest(r *http.Request) ListParams {
+	var lp ListParams
+	var pageIndex int
+	// Limit


no need for these, should be clear from the code. I'll find the article that talks about implicit inline subroutines via comments & send it over 😄

b5 · 2017-11-06T23:20:20Z

core/params_test.go

+
+func ListParamsEqual(a, b ListParams) error {
+	if a.Limit != b.Limit {
+		return fmt.Errorf("ListParams.Limit fields not equal: '%v' != '%v'", a.Limit, b.Limit)


should use %d, for all integer values in fmt.Printf functions. see: https://godoc.org/fmt#hdr-Printing.

b5 · 2017-11-06T23:20:46Z

core/params_test.go

+		return fmt.Errorf("ListParams.Offset fields not equal: '%v' != '%v'", a.Offset, b.Offset)
+	}
+	if a.OrderBy != b.OrderBy {
+		return fmt.Errorf("ListParams.OrderBy fields not equal: '%v' != '%v'", a.OrderBy, b.OrderBy)


should use %s for writing formatted strings, see above

b5 · 2017-11-06T23:21:57Z

core/params.go

+)
+
+const DEFAULT_PAGE_SIZE = 100
+const DEFAULT_LIST_ORDERING = "created"


as mentioned above, let's nix this & just have empty string be default list ordering. Implementations will have to specify what the default order is.

b5 · 2017-11-06T23:23:27Z

core/params.go

+	"created": true,
+	//"modified": true,
+	//"name": true,
+}


very good use of a map for validation! But again, I think we have to leave it up to implementers to decide what a valid order argument is and isn't. Maintaining that list here will frustrate us in the long run

b5 · 2017-11-06T23:24:54Z

core/params_test.go

+		res      ListParams
+		expected string
+	}{
+		// [0]


personal style preference, if we're adding index hints as comments, I'd do it inline as a preceding multiline comment, or lose them entirely. People forget to update these & then they become a source of confusion.

- cleaned up and deleted commented-out commented out lines added in previous pagination commit - added ListParams `Page()` method - updated core/params and core/params to no longer assume a default value of 'created' -

- cleaned up and deleted commented-out lines that were added in previous pagination commit - added ListParams `Page()` method - updated core/params and core/params to no longer assume a default value of 'created'

…to improve_pagination

b5

just a quick convention change to bring the "page" allocation inline. I don't think readers of this code need to have p declared as it's own var

b5 · 2017-11-07T14:59:02Z

api/handlers/datasets.go

-	// core.ListParams rather than a util.Page struct
-	// for time being I added an empty util.Page struct
-	if err := util.WritePageResponse(w, res, r, util.Page{}); err != nil {
+	p = args.Page()


p is undefined here.

b5 · 2017-11-07T14:59:27Z

api/handlers/datasets.go

-	// for time being I added an empty util.Page struct
-	if err := util.WritePageResponse(w, res, r, util.Page{}); err != nil {
+	p = args.Page()
+	if err := util.WritePageResponse(w, res, r, p); err != nil {


I'd just change this to read util.WritePageResponse(w, res, r, args.Page())

b5 · 2017-11-07T15:00:03Z

api/handlers/peers.go

-	// core.ListParams rather than a util.Page struct
-	// for time being I added an empty util.Page struct
-	util.WritePageResponse(w, res, r, util.Page{})
+	util.WritePageResponse(w, res, r, p)


ditto here: util.WritePageResponse(w, res, r, args.Page)

…inline expression

b5

🇲🇴 🐦 🍰

Thomas Osterbind added 2 commits November 6, 2017 17:12

fix: updated handlers functions taking a ListParams to use the OrderB…

1bf6ec3

…y field

ghost assigned osterbit Nov 6, 2017

ghost added the in progress label Nov 6, 2017

b5 reviewed Nov 6, 2017

View reviewed changes

Thomas Osterbind added 3 commits November 7, 2017 09:48

style: ammended pagination update and changed default ListParams.OrderBy

a10eea0

- cleaned up and deleted commented-out lines that were added in previous pagination commit - added ListParams `Page()` method - updated core/params and core/params to no longer assume a default value of 'created'

Merge branch 'improve_pagination' of https://github.com/qri-io/qri in…

3a5bb64

…to improve_pagination

b5 reviewed Nov 7, 2017

View reviewed changes

style: moved calls to args.Page() from a variable assignment to an …

b0a335e

…inline expression

b5 approved these changes Nov 7, 2017

View reviewed changes

osterbit merged commit 4d5647b into master Nov 7, 2017

ghost removed the in progress label Nov 7, 2017

osterbit deleted the improve_pagination branch November 7, 2017 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve pagination #96

fix: Improve pagination #96

osterbit commented Nov 6, 2017

b5 left a comment

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 Nov 6, 2017

b5 left a comment •

edited

Loading

b5 Nov 7, 2017

b5 Nov 7, 2017

b5 Nov 7, 2017

b5 left a comment

fix: Improve pagination #96

fix: Improve pagination #96

Conversation

osterbit commented Nov 6, 2017

b5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b5 left a comment

Choose a reason for hiding this comment

b5 left a comment •

edited

Loading