-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
Unify request/response types at the FacTec API client #120
Conversation
Removing Is there more work planned here? If so, I suggest creating a TODO list in the PR (i.e. like at #38 and #31). I recall we discussed things like having a single common type for the errors, and extracting While reviewing the code, I've noticed some other potential improvements:
|
It would be nice to have smth like this: #[derive(Debug, Deserialize)]
#[serde(tag = "error")]
enum FacetecResponse<T> {
/// Error response variant.
#[serde(rename = true)]
Err(ServerError),
#[serde(rename = false)]
/// Correct response variant.
Ok(T),
} But this serde issue is open since 2017, and this pull request is closed, so I decide to reconsider it a bit. |
This is what we observed so far, but I don't think we can prove it's actually the case. My thinking is this: if our intention is to decide whether it's an error or not is based on the error value - then it is what we have to code. If we encounter a case where My suggestion here would be using the
We might want to add some subclassification for |
921e772
to
203be3a
Compare
…a, CallData and ServerInfo
ea4841b
to
83594a0
Compare
Resolves #70.
To do:
CommonResponse
AlreadyEnrolled
error
field in allResponse
structsFaceScanSecurityChecks
fieldsDeserialize
forFacetecResponse
error_message
and non-error with it