Remove Options on task details fields
bidoubiwa opened this issue · 5 comments
Depending on which async action you are triggering, the Details
fields of a Task can return different information. See spec.
These field are always returned and are not Optional
.
Thus, we should remove the Option. Example:
Lines 57 to 67 in 46d38e6
Should be changed to
#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct IndexCreation {
pub primary_key: String,
}
#[derive(Debug, Clone, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct IndexUpdate {
pub primary_key: String,
}
These field are always returned and are not
Optional
.
According to the doc of indexCreation
and indexUpdate
, primaryKey
can be null
in some situations:
primaryKey
: Value for the primaryKey field into the POST index payload.null
if no primaryKey has been specified at the time of the index creation.
I wonder if Option<String>
was chosen instead of String
for such situations?
Hey @choznerol
Definitely Option<string>
should be used in this specific situation. My example was not the best 😅
Some others are impacted, for example, based on the specs these one:
Lines 64 to 69 in fc1d41d
and probably others that should be checked.
That’s not an absolute source of truth, but this code might help in meilisearch; it’s how we represent the details on disk.
https://github.com/meilisearch/meilisearch/blob/918ce1dd674f99a41545c50272507a275ca53baf/meilisearch-types/src/tasks.rs#L499-L511
Hello,
I had a look at the code and I don't believe this issue is still relevant today. From what I could see, the only place in the code where the Option<> could be removed is TaskType however since TaskType is used for both successedtasks and failedtasks it means there's always a possibility for a Null
return so the Option is necessary.
Let me know if I misunderstood anything.
Cheers!
That’s true; we would need to duplicate the entire TaskType
object or the Succeeded task type into the FailedTask to make something work, and it wouldn’t improve the experience much.
I believe that parts of the issue were fixed in the past, and only the Detail
thing remains but isn’t applicable.
Closing, thanks for investigating it @NoodleSamaChan