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

Add handler factory support #8

Merged
merged 9 commits into from
Aug 15, 2016
Merged

Add handler factory support #8

merged 9 commits into from
Aug 15, 2016

Conversation

ameech
Copy link
Contributor

@ameech ameech commented Aug 15, 2016

Adding handler factories ensures a clean slate when executing each job.

use Equip\Queue\Exception\RouterException;
use Equip\Queue\RouterInterface;

class Router implements RouterInterface
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class needs a test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For an example class?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't notice the path. Feel free to disregard. Though, is this implementation not usable on its own outside of being an example? Are we going to expect that library users will implement their own router classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't plan on providing an implementation, but this one turned out alright. I'll add it.

use Equip\Queue\Exception\HandlerException;
use Equip\Queue\Exception\RouterException;

class SimpleRouter implements RouterInterface

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this called a router and not factory?

use Equip\Queue\Exception\HandlerException;
use Equip\Queue\Exception\RouterException;

class SimpleRouteFactory implements RouteFactoryInterface
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadowhand better?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why is it called "Route" at all?

@ameech ameech changed the title Add RouterInterface Add route factory support Aug 15, 2016
interface RouteFactoryInterface
{
/**
* Retrieve callable for message

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this comment, it seems like this interface should be named HandlerFactoryInterface?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@ameech ameech changed the title Add route factory support Add handler factory support Aug 15, 2016
*/
private $handlers;
private $handler_factory;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$handler, $factory, or leave it as it is?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay as is, though if you do rename it, I'd opt for $handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like $handlers better, updated.


/**
* @param DriverInterface $driver
* @param Event $event
* @param LoggerInterface $logger
* @param MessageSerializerInterface $serializer
* @param RouteFactoryInterface $router
* @param HandlerFactoryInterface $handler_factory

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use $handlers

@shadowhand
Copy link

👍

@ameech ameech merged commit 8607fa9 into master Aug 15, 2016
@ameech ameech deleted the feature/add-router branch August 15, 2016 21:44
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.

3 participants