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

Jump to bottom

Add missing startedAt field in EnqueuedTask #485

Merged
merged 1 commit into from
Sep 11, 2023
Merged

Add missing startedAt field in EnqueuedTask #485

merged 1 commit into from
Sep 11, 2023

Conversation

sebastiantoh
Copy link
Contributor

@sebastiantoh sebastiantoh commented Jun 24, 2023 •
edited
Loading

Pull Request

Related issue

Fixes #462

What does this PR do?

Add missing startedAt field in EnqueuedTask

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

bidoubiwa
bidoubiwa reviewed Jun 26, 2023
View reviewed changes
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

thanks a lot for this contribution 🙏

@bidoubiwa
Copy link
Contributor

Could you give a look at the failing tests?

@sebastiantoh
Copy link
Contributor Author

sebastiantoh commented Jun 26, 2023 •
edited
Loading

Could you give a look at the failing tests?

oops, thanks for pointing it out! i've updated the field to be optional since EnqueuedTasks in Enqueued state do not have a startedAt . Only EnqueuedTasks that are Processing have the startedAt field.
Could you help to re-run the CI?

P.s. would it be better to create a separate ProcessingTask struct instead? Currently, tasks in Enqueued and Processing state both share the EnqueuedTask struct (https://github.com/meilisearch/meilisearch-rust/blob/main/src/tasks.rs#L190-L197)

@bidoubiwa
Copy link
Contributor

P.s. would it be better to create a separate ProcessingTask struct instead? Currently, tasks in Enqueued and Processing state both share the EnqueuedTask struct (https://github.com/meilisearch/meilisearch-rust/blob/main/src/tasks.rs#L190-L197)

I think it would be better! If you have time to implement that go ahead 🙏

@sebastiantoh
Copy link
Contributor Author

thanks for the review! i've made the requested changes

@bidoubiwa
Copy link
Contributor

Hey @sebastiantoh

I'll be reviewing this very soon :) Thanks a lot 🙏

@bidoubiwa bidoubiwa requested a review from irevoire July 18, 2023 12:04
@sebastiantoh
Copy link
Contributor Author

@bidoubiwa @irevoire gentle ping. Happy to make any changes if required :-)

irevoire
irevoire approved these changes Aug 28, 2023
View reviewed changes
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

code-wise it seems good.
I don’t have the time to test it though

curquiza
curquiza approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

bors merge

@meili-bors
Copy link
Contributor

meili-bors bot commented Sep 11, 2023

Build succeeded:

@meili-bors meili-bors bot merged commit e93c27b into meilisearch:main Sep 11, 2023
6 checks passed
@curquiza curquiza added the enhancement New feature or request label Sep 12, 2023
@curquiza curquiza mentioned this pull request Sep 12, 2023
Merged
meili-bors bot added a commit that referenced this pull request Sep 12, 2023
@meili-bors @curquiza
Merge #519
7dd3bd2 
519: Update version for v0.24.2 r=curquiza a=curquiza

## 🚀 Enhancements

* Re-export public structures for better import path (#493) `@adriantombu`
* Added function for sending NDJSON documents (#494) `@carlosb1`
* Add missing startedAt field in EnqueuedTask (#485) `@sebastiantoh`
* #499 (attributesToSearchOn) (#510) `@brendon-shf`
* Add total field to get-tasks route (#509) `@hmacr`
* Implement client for experimental-features API (#516) `@hmacr`


Co-authored-by: curquiza <clementine@meilisearch.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bidoubiwa bidoubiwa bidoubiwa left review comments

@irevoire irevoire irevoire approved these changes

@curquiza curquiza curquiza approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tasks in state Processing do not have startedAt time
4 participants
@sebastiantoh @bidoubiwa @irevoire @curquiza