Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add purge to ProducerInterface#16

Merged
sjparkinson merged 3 commits intomastergraze/queue:masterfrom
feature/purge-queuegraze/queue:feature/purge-queueCopy head branch name to clipboard
Aug 28, 2015
Merged

Add purge to ProducerInterface#16
sjparkinson merged 3 commits intomastergraze/queue:masterfrom
feature/purge-queuegraze/queue:feature/purge-queueCopy head branch name to clipboard

Conversation

@sjparkinson
Copy link
Copy Markdown
Contributor

Initial draft for resolving #14.

Depends on / based off #15.

Adds the purge method to ProductInterface.

@sjparkinson sjparkinson modified the milestone: v0.1.2 Aug 18, 2015
@sjparkinson sjparkinson force-pushed the feature/purge-queue branch 3 times, most recently from d8228ac to dc5bef3 Compare August 18, 2015 17:22
@sjparkinson sjparkinson force-pushed the master branch 2 times, most recently from d626d09 to 97cab30 Compare August 18, 2015 17:29
Comment thread src/Adapter/AdapterInterface.php Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the less specific PHPDoc argument?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Personal preference as the type hint is for array, but happy to undo.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ah, I see. I'm sure it's fine then.

@mathieupinet
Copy link
Copy Markdown

looks good otherwise (not tested).

@sjparkinson sjparkinson force-pushed the feature/purge-queue branch 2 times, most recently from e936770 to 86956d0 Compare August 20, 2015 15:39
Comment thread src/ProducerInterface.php
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Given the talk today, any thoughts on this @wpillar?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here are my thoughts:

  • Client implements ProducerInterface and ClientInterface, so adding purge() to ProducerInterface does not unreasonably affect that class.
  • You've already got segregated interfaces in ProducerInterface and ClientInterface and since Client implements both, the only consideration really is how people might extend this code. Are we burdening people who might write separate ProducerInterface implementations with a method they can't implement? My thinking here is if we could reasonably assume that all queue implementations have a way of purging then it's fine to put this on ProducerInterface, as a purge operation is similar to a send operation. If we think this assumption is unreasonable then you can segregate further with a PurgeableInterface.

What's your thinking on this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My thinking here is if we could reasonably assume that all queue implementations have a way of purging then it's fine to put this on ProducerInterface, as a purge operation is similar to a send operation.

So far it's 👌, but I doubt every queue implementation supports purging.

We're not 1.0.0, so I'm going to go with this and see what Mat thinks if he makes use of it.

sjparkinson added a commit that referenced this pull request Aug 28, 2015
@sjparkinson sjparkinson merged commit 4226d07 into master Aug 28, 2015
@sjparkinson sjparkinson deleted the feature/purge-queue branch August 28, 2015 13:26
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.