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.

Sign up for GitHub

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

Add a Sort RequestOption helper #298

Merged
merged 2 commits into from
Oct 25, 2023
Merged

Add a Sort RequestOption helper #298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a47d2d0
Add a Sort RequestOption helper
ewanharris Oct 24, 2023
89e6df0
Merge branch 'main' into feat/sort-helper
ewanharris Oct 25, 2023
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
1 change: 1 addition & 0 deletions EXAMPLES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ management.PerPage()
management.IncludeTotals()
management.Take()
management.From()
management.Sort()
```

## Pagination
Expand Down
10 changes: 10 additions & 0 deletions management/management_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,3 +285,13 @@ func Stringify(v interface{}) string {
}
return string(b)
}

// Sort configures a request to sort data by the selected field.
// Use 1 to sort ascending and -1 to sort descending.
func Sort(sort string) RequestOption {
Copy link
Contributor

@Widcket Widcket Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we use a string parameter for the field, and a boolean for the sort direction? Instead of expecting the exact string that the underlying API endpoint expects.

Copy link
Contributor Author

@ewanharris ewanharris Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd noodled around with this a bit, initially I had Sort("name", "-1") then I looked at Java/.Net and they had a style like what I went with (Sort("name:-1")) but I'm on the fence about it as they all kinda hide what the real meaning of 1/-1 are so on a scan through usage it's not immediately obvious what's being performed.

I did consider doing SortAscending("name") and SortDescending("name") to make it clearer but figured two might be overkill, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that "1" and "-1" don't really express much about what they mean.

I did consider doing SortAscending("name") and SortDescending("name") to make it clearer

How are we already handling similar cases in the public API, if any?
BTW these seem quite readable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't really anything too like this unfortunately, I guess the closest might the the Query API allowing passing similar strings

return newRequestOption(func(r *http.Request) {
q := r.URL.Query()
q.Set("sort", sort)
r.URL.RawQuery = q.Encode()
})
}
11 changes: 11 additions & 0 deletions management/management_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,17 @@ func TestOptionDefaults(t *testing.T) {
assert.Equal(t, "false", includeTotals)
}

func TestOptionSort(t *testing.T) {
r, _ := http.NewRequest("GET", "/", nil)

Sort("name:-1").apply(r)

v := r.URL.Query()

sort := v.Get("sort")
assert.Equal(t, "name:-1", sort)
}

func TestStringify(t *testing.T) {
expected := `{
"foo": "bar"
Expand Down
Loading