-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add a Sort RequestOption helper #298
Conversation
639e171
to
a47d2d0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
=======================================
Coverage 94.80% 94.80%
=======================================
Files 46 46
Lines 8910 8916 +6
=======================================
+ Hits 8447 8453 +6
Misses 361 361
Partials 102 102
☔ View full report in Codecov by Sentry. |
|
||
// 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
🔧 Changes
Adds a
Sort
helper function that can be used to configure the field to sort by and direction.Closes #221
📚 References
🔬 Testing
📝 Checklist