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

Operators #28

Merged
merged 7 commits into from
Jun 19, 2022
Merged

Operators #28

merged 7 commits into from
Jun 19, 2022

Conversation

SamuelMwangiW
Copy link
Contributor

@SamuelMwangiW SamuelMwangiW commented Jun 13, 2022

Thanks for the amazing work 👏🏽.

This PR proposes to introduce an decorator pattern to customize the request and response parameters to suit each service provider.

Am also not sure of the names and went Operator as you call them operators in the docs. I was also unsure interface method name therefore I welcome suggestions besides decorate.

This allows the developer to define their operator in the config the init the machine in the controller as the docs and everything just works TM.

The Operator only needs to implement Sparors\Ussd\Contracts\OperatorContract::class. Samples from #22 (comment) are provided but the developer is at liberty to use their own.

If properly set, the following should just workTM.

use Sparors\Ussd\Facades\Ussd;

$machine = Ussd:machine()
            ->setInitialState(HelloState::class);

return response()->json($machine->run());

This has the added benefit that the same code can be used for different providers and the configured provider is dynamically set in say a middleware 💪

Tests might need to improvement therefore any help is welcome.

Once approved, kindly squash merge 😜

@SamuelMwangiW SamuelMwangiW marked this pull request as ready for review June 14, 2022 08:56
@SamuelMwangiW
Copy link
Contributor Author

No more commits, should be ready for your review.

Feel free to close if you have reservations about the PR.

Thanks once again for an amazing package 👏

@papamarfo
Copy link
Member

Nice work man. I like the idea of the abstraction and supporting multiple providers as well. Why leave the test for us to write though 😂. Just joking

@SamuelMwangiW
Copy link
Contributor Author

Nice work man. I like the idea of the abstraction and supporting multiple providers as well. Why leave the test for us to write though 😂. Just joking

Thanks buddy for the kind words. I had the feeling that tests were incomplete at the time of writing the comment but has since improved the situation with 6384581 and are open to add tests for any uncovered areas.

@cybersai
Copy link
Member

Thanks, @SamuelMwangiW for your contribution. However, I would like to remove implementations for operators in the library to prevent us from having to maintain that in the future. Since this is not a major version, I would like for us to prevent making changes to the config file. I would therefor merge this to a different branch and do the changes and let you review if you are okay with it.

@cybersai cybersai changed the base branch from main to operators June 19, 2022 10:04
@cybersai cybersai merged commit ac15d46 into sparors:operators Jun 19, 2022
@SamuelMwangiW SamuelMwangiW deleted the operators branch June 19, 2022 10:07
@SamuelMwangiW
Copy link
Contributor Author

Thanks @cybersai for your review.
I also had reservations about the changes to the config file. I would want to see suggestions from developers whose applications are integrating with more than one operator on how they would go about it then we can merge the branch into the next major.
I will however be using the operators branch as it makes integrating with a single operator a breeze.
Kudos for your work on this amazing package

@cybersai
Copy link
Member

Thanks, @SamuelMwangiW, you can check #29 and submit your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants