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

[Context] Separate validate() and execute() handlers for better modularity and API coherence #539

Closed
Tracked by #554
Farhad-Shabani opened this issue Mar 16, 2023 · 2 comments · Fixed by #661
Closed
Tracked by #554
Assignees
Labels
A: breaking Admin: breaking change that may impact operators O: decoupling Objective: aims to separate concerns and cause to independent, reusable components
Milestone

Comments

@Farhad-Shabani
Copy link
Member

Problem Statement

The ValidationContext and ExecutionContext traits now include default implementations for their validate() and execute() methods. However, these methods use functions that are only public to the crate. This makes it nearly impossible for someone to use a different implementation and renders keeping them as API meaningless.

Moreover, looking at the operations they do, it does not make sense to keep these two (validate/execute) along with the others. They deal with the handler functionality and route an incoming message to the appropriate validation/execution handler, while others mainly interact with storage for retrieving or setting a state. It would be better to keep these two roles separate to achieve more modularity.

@Farhad-Shabani Farhad-Shabani added A: breaking Admin: breaking change that may impact operators O: decoupling Objective: aims to separate concerns and cause to independent, reusable components labels Mar 16, 2023
@plafer
Copy link
Contributor

plafer commented Mar 16, 2023

IIUC, you're suggesting to remove validate() and execute() from the traits, right? I agree with that, they are not meant to ever be re-implemented, so they should live outside the trait. They should instead live as free functions such as these ones (which I'm not sure if we need to keep).

@Farhad-Shabani
Copy link
Member Author

Yes. exactly, something like free functions.

I also had an idea to fix this issue before (check out the second idea here). It involved using ValidationHandler and ExecutionHandler traits (ofc only public to the crate) across a context that implements ValidationContext and/or ExecutionContext.
I liked that since it implicitly was differentiating the role of each Handler and Context instance with respect to an incoming message.

I'll give it another look and see if it still holds up, I'll work on refining that when I'm back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: breaking Admin: breaking change that may impact operators O: decoupling Objective: aims to separate concerns and cause to independent, reusable components
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants