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.

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

Refactor ResponseStatus #56

Open
ionut-arm opened this issue May 29, 2020 · 6 comments
Open

Refactor ResponseStatus #56

ionut-arm opened this issue May 29, 2020 · 6 comments
Labels
enhancement New feature or request medium Effort label

Comments

@ionut-arm
Copy link
Member

The ResponseStatus enum is currently defined as a monolithic enum holding all possible values. While not entirely wrong, the usability of the type could be improved by re-arranging the variants into logical groups. This would also entail a refactoring of all types related to error management.

Proposed changes:

  • a Result<T> type would still be available, however the error type would not be a ResponseStatus but a new ErrorStatus type; this is to prevent a logical error of having an error of type Success
  • thus, ResponseStatus will be an enum with two variants: Success and Error(ErrorStatus)
  • ErrorStatus will be further split into Service(ServiceError) (for internal service-related errors) and Psa(Error) (for errors related to the functionality of Parsec, generally returned from providers) - the error contained within the variant will be directly drawn from the psa-crypto Rust crate
@ionut-arm ionut-arm added the enhancement New feature or request label May 29, 2020
@ionut-arm ionut-arm self-assigned this May 29, 2020
@hug-dev
Copy link
Member

hug-dev commented May 29, 2020

Agree with those three points! To lay it down:

type Result<T> = Result<T, ErrorStatus>;

enum ResponseStatus {
    Success,
    Error(ErrorStatus),
}

enum ErrorStatus {
    Service(ServiceError),
    Psa(psa_crypto::types::status::Error),
}

enum ServiceError {
    WrongProviderID,
    ...
}

With From implementations from the wrapper type to the outer type.

That would also mean that we lose the automatic conversion to the integer representation but that is fine I think. We can implement a method for that, it would be cleaner as well in my opinion.

Last point: do we agree on the names of each type? Do we want to use namespacing to trim some of those names? ErrorStatus could be Error for example as it is in parsec_interface::requests::response_status. Not sure.

@ionut-arm
Copy link
Member Author

Last point: do we agree on the names of each type? Do we want to use namespacing to trim some of those names? ErrorStatus could be Error for example as it is in parsec_interface::requests::response_status.

I think that makes sense!

@ionut-arm
Copy link
Member Author

How about we drop the ResponseStatus altogether, since it adds nothing? No one would use ResponseStatus because what we return (and what we should return) from all our client methods and within the service is Result<T, Error>. If you want, we can rename the type to pub type ResponseStatus<T> = Result<T, Error>.

@hug-dev
Copy link
Member

hug-dev commented Jun 4, 2020

what we return (and what we should return) from all our client methods and within the service is Result<T, Error>

Totally agree with that!

I would think that having a ResponseStatus type is useful to serialise/deserialise the corresponding value in the requests/responses? We can have it just for that purpose but using Result<T, Error> everywhere?

@ionut-arm
Copy link
Member Author

It seems the problem with the type hierarchy described in the first comment is that of converting from u16 to a final usable version of the enums. Overall I'd say we end up doing at least as much work for not much gain, so I'm dropping this for now - maybe we'll manage to find an alternative that works in the future.

@hug-dev hug-dev added this to the Parsec production ready milestone Aug 7, 2020
@hug-dev hug-dev removed this from the Parsec production ready milestone Sep 2, 2020
@hug-dev hug-dev added the medium Effort label label Feb 3, 2021
@hug-dev
Copy link
Member

hug-dev commented Feb 3, 2021

To be done with #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request medium Effort label
Projects
None yet
Development

No branches or pull requests

2 participants