Conversation
brandur
left a comment
brandur
left a comment
There was a problem hiding this comment.
Thanks @thatjos! Looking very good.
Did a preliminary review here, but @bgentry, since you wrote the bulk of this code you should take the final review.
Notably: we've got a couple breaking changes in here. I'm on the fence — we're trying not to do that, but on the other hand, the job list API is still quite new, so maybe we could get away with it. What do you think?
| // // handle error | ||
| // } | ||
| func (c *Client[TTx]) JobList(ctx context.Context, params *JobListParams) ([]*rivertype.JobRow, error) { | ||
| func (c *Client[TTx]) JobList(ctx context.Context, params *JobListParams) ([]*rivertype.JobRow, *JobListCursor, error) { |
There was a problem hiding this comment.
Agreed that the code needed to get the next cursor is a little overly verbose at the moment, and furthermore you have to make a len check on jobs to make sure you're not indexing a 0-length slice.
jobs, err := client.JobList(ctx, NewJobListParams().After(JobListCursorFromJob(jobs[len(jobs)-1])))If we were going to change this API though, I think I'd be tempted to make it more like a result struct, which is a bit more conventional, and a little more futureproof. e.g.
type JobListResult struct {
Jobs []*rivertype.JobRow
NextCursor *JobListCursorThoughts?
| } | ||
| case JobListOrderByScheduledAt: | ||
| time = job.ScheduledAt | ||
| } |
There was a problem hiding this comment.
OOC, wasn't this switch in jobListTimeValue before (and isn't jobListTimeValue still in this file?)? Is moving it up just to remove a layer of indirection?
There was a problem hiding this comment.
Yeah, unfortunately the cursor is tied to the sorting mechanism that was used for the initial query. If the sorting mechanism was the generic 'time' one (meaning - inferred through the state that was queried), we'd have to do the same thing here. If it wasn't, and the user sorted by a specific timestamp field, that needs become a part of the cursor. The logic is slightly different between this function and jobListTimeValue, and I couldn't think of a way to combine the functions. Happy to edit if you have thoughts!
|
Oh and @thatjos, you may want to pull down golangci-lint and get that running locally. A few lint errors here, and the local tool makes it much faster to find/fix them. Depending on your OS flavor, something like: $ brew install golangci-lint
$ golangci-lint run --fix |
|
Agreed on all points, I'll clean it up! |
|
@thatjos Thanks! Looking good. I have a few nit-picky comments, but will wait until @bgentry's able to do a higher level review first so that you don't end up making changes that weren't needed. |
|
@thatjos sorry for the delay on this, I've had to prioritize fixing the memory leaks these past few days. I will get to it soon! |
No worries, thanks so much for the great work |
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
LGTM aside from a couple minor nits. We'll need a breaking changelog entry for this as well and will want to bump the minor prerelease version to indicate the break.
Thanks for your patience!
| // jobs and a cursor for fetching the next page of results. | ||
| type JobListResult struct { | ||
| Jobs []*rivertype.JobRow | ||
| Cursor *JobListCursor |
There was a problem hiding this comment.
Not sure it makes sense to output a single Cursor field here. There could be a cursor for any of the entries in the Jobs list, so with this name it's not clear if it's the start cursor or end cursor.
I doubt there's much use case for the start cursor, and people can always use the JobListCursorFromJob function to create one if they want. Maybe let's just name this LastCursor to be unambiguous?
| // JobListOrderByTime specifies that the sort should be by time. The specific | ||
| // time field used will vary by job state. | ||
| JobListOrderByTime JobListOrderByField = iota | ||
| // time field used will vary by the first specified job state. | ||
| JobListOrderByTime JobListOrderByField = "time" | ||
| // JobListOrderByCreatedAt specifies that the sort should be by created_at. | ||
| JobListOrderByCreatedAt JobListOrderByField = "created_at" | ||
| // JobListOrderByScheduledAt specifies that the sort should be by scheduled_at. | ||
| JobListOrderByScheduledAt JobListOrderByField = "scheduled_at" | ||
| // JobListOrderByAttemptedAt specifies that the sort should be by attempted_at. | ||
| JobListOrderByAttemptedAt JobListOrderByField = "attempted_at" | ||
| // JobListOrderByFinalizedAt specifies that the sort should be by finalized_at. | ||
| JobListOrderByFinalizedAt JobListOrderByField = "finalized_at" |
There was a problem hiding this comment.
Can we keep these sorted alphabetically?
|
Thanks so much @bgentry and apologies for the delay! Fixed up those things and wrote an unreleased changelog entry. Would you be able to help me cut a new release whenever's convenient for you? |
|
Added "breaking change" label. Sorry for the delay here @thatjos. Have spoken about this one with Blake, and we'll get back to you soon. We're going to ship a couple small breaking changes as part of a single release to make sure that we're not making upgrade headaches a regular feature. See also #292. |
|
@thatjos Blake ended up pulling your commits into another PR in #304 which we merged in with a few tweaks. We should have a release cut sometime this weekend. Closing this PR out, but thanks for the contribution. |
3 participants
This supersedes #227. It adds: