Sort Order for tasks #110

konrad · 2019-12-03T20:26:15Z

konrad commented

2019-12-03 20:26:15 +00:00

Description

Checklist

I added or improved tests
I pushed new or updated dependencies to the repo using go mod vendor
I added or improved docs for my feature
- Swagger (including make do-the-swag)
- Error codes

# Description # Checklist * [x] I added or improved tests * [x] I pushed new or updated dependencies to the repo using `go mod vendor` * [x] I added or improved docs for my feature * [x] Swagger (including `make do-the-swag`) * [x] Error codes

konrad reviewed 2019-12-03 22:02:05 +00:00

pkg/models/tasks.go Outdated

						
				@ -257,0 +324,4 @@

						// This is a descending sort, so we need to negate the comparator (i.e. switch the inputs).

						if param.orderBy == "desc" {

							oldComparator := comparator

konrad commented

2019-12-03 22:02:05 +00:00

@shilch Why are you duplicating the comparator here?

shilch commented

2019-12-03 22:47:09 +00:00

If I didn't you would end in an infinite recursive loop because comparator would call itself.

konrad reviewed 2019-12-03 22:04:45 +00:00

pkg/models/tasks.go Outdated

						
				@ -257,0 +319,4 @@

					for _, param := range by {

						comparator, ok := comparators[param.sortBy]

						if !ok {

							// TODO: Handle case that a suitable comparator has not been found

konrad commented

2019-12-03 22:04:45 +00:00

IMHO we could panic here, this is only an internal api. User validation input happens way earlier.

konrad reviewed 2019-12-03 22:24:14 +00:00

pkg/models/task_collection.go Outdated

						
				@ -37,0 +46,4 @@

					sortProperty string

					sortOrder bool

konrad commented

2019-12-03 22:24:07 +00:00

Any special resons why this is a bool? I know there can only be two types of sort orders, but a bool here is IMHO a bit overkill since bool has nothing to do with sort order.

shilch commented

2019-12-03 22:49:30 +00:00

What would you use instead? Note how it's not aliased but typed, i.e. you can not accidentally convert one to the other.

konrad commented

2019-12-04 16:57:58 +00:00

Makes sense, thanks for clarifying.