Put type on timestamp and duration #237
Conversation
|
Oops sorry I created this PR a little bit too early |
irevoire
force-pushed
the
time
branch
2 times, most recently
from
e5780b5 to
18255b1
Compare
|
Now it’s ready to review 👍 |
| [dependencies] | ||
| async-trait = "0.1.51" | ||
| serde_json = "1.0" | ||
| iso8601-duration = "0.1.0" |
There was a problem hiding this comment.
Sorry for the newbie here 😅
I think I get the idea, but just to be sure, the users of the rust-sdk don't need to have these time-handling libraries on their apps?
In other words, to be able to use some function they need to have the lib, in order to have the right types to pass to the function?
There was a problem hiding this comment.
The iso8601-duration crate is never exposed to the end-user, but sadly the time crate will be needed if someone wants to manipulate API keys.
For the context, there was a lot of TODO: use chrono (which is one of the biggest time crates in the rust ecosystem).
And there was this comment asking for a real « time » type instead of String in the SDK: meilisearch/integration-guides#121 (comment)
And the reason why we’re not using chrono is here: #226
There is a bunch (2 or 3, I think) of CVE on chrono that are not fixed because the crate is not really maintained anymore.
But for our use case, chrono was just a wrapper around time (which is maintained and fixed all of the CVE encountered in chrono), so I decided to import this one instead of chrono.
Also, time is way smaller than chrono, and you can configure a lot of features depending on your needs.
But yes, to conclude, a user of the meilisearch-rust sdk can do most of the thing without any problem, but if he wants to use inspect, create or update anything related to the OffsetDateTime, he’ll need to import time in his crate.
apply review comment
There was a problem hiding this comment.
It looks good to me, thank you @irevoire!
Will let @brunoocasali merge it then :)
curquiza
added
the
breaking-change
|
I put the breaking change label since it impacts the user experience (cf code sample changes) 🙂 |
|
bors merge |
237: Put type on timestamp and duration r=brunoocasali a=irevoire Parse the Duration and timestamp with the `iso8601-duration` and the `time` crate Co-authored-by: Irevoire <[email protected]>
|
This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried. Additional information: {"message":"At least 1 approving review is required by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"} |
|
bors merge |
|
Build succeeded: |
243: Update version for the next release (v0.15.0) r=brunoocasali a=brunoocasali ##⚠️ Breaking changes * Refactorise the errors, now `error_code` and `error_type` has types instead of `string` (#234) `@irevoire` * Put type on timestamp and duration (#237) `@irevoire` * The `time` crate will be needed if you need to manipulate API keys (check for more info #226, #237 (comment), and meilisearch/integration-guides#121 (comment)). ## 🚀 Enhancements * Refactorise the settings (#235) `@irevoire` Thanks again to `@irevoire!` 🎉 Co-authored-by: Bruno Casali <[email protected]>
4 participants
Parse the Duration and timestamp with the
iso8601-durationand thetimecrate