Improve checkpoint pagination support #179
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
=======================================
Coverage 94.88% 94.88%
=======================================
Files 38 38
Lines 6807 6810 +3
=======================================
+ Hits 6459 6462 +3
Misses 278 278
Partials 70 70
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
ewanharris
changed the title
| if l.Next != "" { | ||
| return true | ||
| } | ||
|
|
||
| return l.Total > l.Start+l.Limit |
There was a problem hiding this comment.
Even though we could rewrite this to simply return l.Next != "" || l.Total > l.Start+l.Limit I prefer how you've done it so it's easier to understand what's happening and is less cluttered 👍🏻
| @@ -156,10 +160,10 @@ func (o *requestOption) apply(r *http.Request) { | |||
| func applyListDefaults(options []RequestOption) RequestOption { | |||
| return newRequestOption(func(r *http.Request) { | |||
| PerPage(50).apply(r) | |||
| IncludeTotals(true).apply(r) | |||
There was a problem hiding this comment.
Nice 👍🏻 this now allows us to also set include totals to false if needed, otherwise it would've been impossible to do so.
| @@ -155,7 +155,7 @@ func TestOptionDefaults(t *testing.T) { | |||
| assert.Equal(t, "20", perPage) | |||
|
|
|||
| includeTotals := v.Get("include_totals") | |||
| assert.Equal(t, "true", includeTotals) | |||
| assert.Equal(t, "false", includeTotals) | |||
There was a problem hiding this comment.
Totally fine to keep it like this, however there's also the assert.False helper func on the assert pkg.
ewanharris
force-pushed
the
fix/issue-109-checkpoint-pagination
branch
from
44e1f0d
to
dbfa976
Compare
|
Added some better redacting of logs and rebased/force pushed to remove some info |
Note that even if you do this, IIRC the commits are still accessible on GitHub if you have the hash. |
|
If any secrets have been pushed (not sure, just in case), the only thing to do is to rotate them. |
🔧 Changes
Fixes checkpoint pagination and adds tests to the APIs that support it
fromparameter here is an ID for a log, not the value returned from the API📚 References
Fixes #109
🔬 Testing
Unit tests added, and pushed the tests before the fix to demonstrate the fix
📝 Checklist