-
Notifications
You must be signed in to change notification settings - Fork 19
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
Roadmap for 3.0 #39
Comments
There is a place I would like to see improved. It confused me when I first contributed to Disque-php and it still confuses me. I think it's hard to maintain and to add new code. It's the way how Commands, Arguments and Responses are structured. Commands contain
Going by the list, there are several points that could be improved in my opinion. Ad 2. Options + 3. Command arguments + 4. Validation I suggest moving the whole code around Disque-php commands and arguments, their validation and their mapping to Disque arguments to their own object. addJob(string $queue, string $payload, array $options = []) one would call it like this: $addJobArguments = new AddJobArguments($queue, $payload); // These arguments are always required
$addJobArguments->setTimeout($timeout); // These arguments are optional
$addJobArguments->setDelay($delay);
$disque->addJob($addJobArguments); // Go Advantages
I also wouldn't use traits at all as they make the code more confusing than composition, in my opinion (and even more so when traits are hierarchical in that they also use other traits). Ad 4. Validation Some method names used in validation could be more explicit.
Similarly the methods in Ad 5. A response interpreter I would inject the proper interpreter explicitly into the command instead of instantiating it inside from a class name new AddJob(new AddJobResponse()) I would clean up the inheritance tree of responses. It's too complicated and used for code reuse, which might be done better with composition. Smaller stuff
All in all I would avoid the pattern, where constants and properties in a class change the behavior of the class. That's probably the most confusing thing in Commands, I find. Instead I would make all behavior explicit, and if shared, shared explicitly by composition. Hopefully the suggestions are understandable, I wrote them more or less as a stream of thoughts. What do you think about this? Regarding the move to PHP7, I support it. The new language features could help eg. with some of the validation. |
I like your approach to command arguments, and indeed validation would be much simpler with scalars. I too feel uneasy about how things are structured now, so I'm open to improving it. Great suggestions @Revisor |
@dominics done |
I'd like to release 3.0 soon. On the list of to-do I see:
-LEAVING
error inGETJOB
and other commands #28I'd also like to make php7 the minimum requirement, and consequently move to full strict typing.
What are your feelings retarding this roadmap and particularly the new minimum php version to be supported?
The text was updated successfully, but these errors were encountered: