Update to use equip/command #9
Conversation
|
@equip/contributors care to take a look? |
| use Equip\Command\OptionsSerializerTrait; | ||
| use Equip\Queue\Exception\MessageException; | ||
|
|
||
| abstract class AbstractMessage implements OptionsInterface |
There was a problem hiding this comment.
Why is the message used as the options?
There was a problem hiding this comment.
Can be renamed, it was when I previous had it as an interface. Will update.
ameech
force-pushed
the
feature/use-commands
branch
from
87c8843
to
1ab1520
Compare
| public function command() | ||
| { | ||
| if (!$this->command) { | ||
| throw MessageException::missingProperty('command'); |
There was a problem hiding this comment.
I don't love the validation happening so late, any ideas?
There was a problem hiding this comment.
Don't think there's really a way to make it happen earlier without adding a constructor that handles validation.
|
I don't really understand the implementation as suggested. Why isn't it more akin to...? $queue->add(FooCommand::class, new FooOptions(...));Currently it looks like you need to wrap the command/options within a message object (which implements options for some reason?) and then pass that to the queue. The current implementation confuses me. Can you explain what the intent here is, @ameech ? |
| use Equip\Queue\Exception\CommandException; | ||
| use Equip\Queue\Exception\HandlerException; | ||
|
|
||
| interface CommandFactoryInterface |
| use Equip\Command\CommandInterface; | ||
| use Equip\Queue\Exception\CommandException; | ||
|
|
||
| class AurynCommandFactory |
There was a problem hiding this comment.
Is this supposed to implement something?
ameech
force-pushed
the
feature/use-commands
branch
from
7e117ab
to
f2b7d61
Compare
|
Looks pretty good, just a couple of little things. |
|
👍 |
| "league/event": "^2.1", | ||
| "psr/log": "^1.0" | ||
| "psr/log": "^1.0", | ||
| "rdlowrey/auryn": "^1.4" |
There was a problem hiding this comment.
Not sure we want to require this versus suggesting it / including it as a dev dependency.
There was a problem hiding this comment.
Since we only have AurynCommandFactory and no CommandFactoryInterface, I figured it's best to add it.
It's back!
This reverts commit 53d750f.
|
@ameech Only think I might suggest is to add a section to the README on implementing |
|
👍 |
|
I plan to come back and make the docs better, they're pretty weak right now. |
Using commands will simplify adding messages to a queue as well as allow us to typehint better within the handlers.
Also, adding this will allow you to convert an
equip/commandversion 2 command to a background job quite easily.TODO