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

adds a noop client which implements all interfaces but … #289

Merged
merged 2 commits into from Jul 9, 2016

Conversation

@gsdevme
Copy link
Contributor

gsdevme commented Jun 29, 2016

As discussed here FriendsOfSymfony/FOSHttpCacheBundle#293

Each method is implemented from the interfaces and follows the docblocks, also using a constructor similar to the abstract. I can't write any tests for this though as phpunit is not included within composer require-dev :O

@gsdevme gsdevme force-pushed the gsdevme:feat/adds-a-noop-client branch 2 times, most recently from ee6df0a to e17ee00 Jun 29, 2016
*
* @author Gavin Staniforth <gavin@gsdev.me>
*/
class Noop implements ProxyClientInterface, BanInterface, PurgeInterface, RefreshInterface, TagsInterface

This comment has been minimized.

Copy link
@dbu

dbu Jun 29, 2016

Contributor

for PSR-2 class loading, class name and file name must match. to be consistent, please rename the file to Noop.php

This comment has been minimized.

Copy link
@gsdevme

gsdevme Jun 29, 2016

Author Contributor

Oh shoot! Copy/Paste. Good spot.

use Http\Message\MessageFactory;

/**
* Class Noop

This comment has been minimized.

Copy link
@dbu

dbu Jun 29, 2016

Contributor

please explain in this doc block that this client is to avoid errors when some environments don't have a caching proxy.

@dbu dbu added the enhancement label Jun 29, 2016
@dbu dbu changed the title feat(client): adds a noop client which implements all interfaces but … adds a noop client which implements all interfaces but … Jun 29, 2016
@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Jun 29, 2016

thanks a lot!

regarding phpunit tests: we use the phpunit that is installed on travisci. locally you can either use a globally installed phpunit or download the phpunit.phar into your project. i know its an ongoing debate whether phpunit should be in require-dev or not...
in this case, i feel we don't have the foundations to add a good test. the only meaningful test i can think of would be a base proxy client test that checks that all methods that are supposed to be chainable indeed return the client object, and run that test with each client implementation. if you want to write that, please do, but i can live without one in this case.

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jun 29, 2016

I have a second PR to add phpunit to require-dev shall I hold off with that? My test was just going to ensure the docblocks are enforced in terms of types.

@gsdevme gsdevme force-pushed the gsdevme:feat/adds-a-noop-client branch 2 times, most recently from 4156733 to 6af7daa Jun 29, 2016
@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Jun 29, 2016

given how much trouble BC breaks in phpunit major version changes are (and that travisci just updates phpunit without our control), i would be ok to have it in require-dev, actually. and yes, such a test makes sense - most sense is a base conformity test that is extended for each client to ensure that all of them act according to the interface doc. but if you don't want to go there, a PR for just the test on Noop is good to, i might do the generalization later.

@gsdevme gsdevme force-pushed the gsdevme:feat/adds-a-noop-client branch from 88fe8cb to b526857 Jun 29, 2016

public function testBan()
{
$this->assertInstanceOf(BanInterface::class, $this->noop->ban(['header-123']));

This comment has been minimized.

Copy link
@dbu

dbu Jun 30, 2016

Contributor

i think we should assertSame($this->noop, $this->noop->ban...) here. the contract specifically says we return the client itself, not any ban capable client.

This comment has been minimized.

Copy link
@gsdevme

gsdevme Jun 30, 2016

Author Contributor

Totally agree here, my IDE picked up $this as that and as I like a train monkey I just did it. Will sort later today.

@gsdevme gsdevme force-pushed the gsdevme:feat/adds-a-noop-client branch 2 times, most recently from 5ca958b to 4d14992 Jul 7, 2016
@gsdevme gsdevme force-pushed the gsdevme:feat/adds-a-noop-client branch from 4d14992 to 847d8f8 Jul 7, 2016
@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 7, 2016

@dbu Rebased with the changes you have merged into master.

@wickedOne

This comment has been minimized.

Copy link
Contributor

wickedOne commented Jul 7, 2016

a bit late to the party. probably nitpicking and possibly not relevant as this in the library rather than the bundle, but wouldn't it make more sense to call it a NullClient, like the NullOutput, NullContext etc.?

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 7, 2016

Not really, I don't feel like NULL has anything to do with this, its a no operation client.

@wickedOne

This comment has been minimized.

Copy link
Contributor

wickedOne commented Jul 7, 2016

fair enough...

as said, just a suggestion: using a Null[something] is a common practice in Symfony components and as this is a new feature i thought i'd thow it in here

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 7, 2016

Yeah I've noticed Symfony does prefer to use NULL which is a little odd, noop as a concept has existed since forever.. Not that Im really against someone changing this.

In computer science, a NOP or NOOP (short for No Operation) is an assembly language instruction, programming language statement, or computer protocol command that does nothing.

Some cases of noop
https://api.jquery.com/jquery.noop/
https://docs.angularjs.org/api/ng/function/angular.noop

Another argument for keeping it is thinking forward to the bundle the configuration would say null under the ProxyClient could lead to confusion perhaps?

@wickedOne

This comment has been minimized.

Copy link
Contributor

wickedOne commented Jul 7, 2016

personally i think it's a matter of taste. noop makes perfect sense. my remark was regarding the symfony way of dealing with this and as said, this is the lib so not sure whether we should take that into account.

at the moment especially the config_test.yml files of the applications i'm working on contain quite a bit of NullHandler and NullOutput settings.
so for me personally a NullClient would make more sense, but once more probably a matter of taste...

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 7, 2016

Calling it NullClient would mean we would have to rename Varnish to VarnishClient and Symfony to SymfonyClient also though? it would have to be named Null to keep it consistent and naming something Null isn't possible.

@calumbrodie

This comment has been minimized.

Copy link
Contributor

calumbrodie commented Jul 8, 2016

I'm 👍 for NullClient FWIW

Edit: Although the reasoning above about having to call the client Client/Null' also makes sense, so calling it NullClient would be a bit inconsistent. ¯_(ツ)_/¯

use Http\Message\MessageFactory;

/**
* Class Noop

This comment has been minimized.

Copy link
@dbu

dbu Jul 8, 2016

Contributor

this line does not add information. i'd rather just have the second part, which explains very well what this class does.

@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Jul 8, 2016

seems indeed a matter of taste to me. Noop says well what this does, lets stick with that.

@gsdevme i added a comment about the docblock, and please check styleci, there are some unused use statements. when you cleaned those up, i will merge.

@gsdevme gsdevme force-pushed the gsdevme:feat/adds-a-noop-client branch from 847d8f8 to b30e074 Jul 8, 2016
@gsdevme gsdevme force-pushed the gsdevme:feat/adds-a-noop-client branch from b30e074 to 3ad7bf6 Jul 8, 2016
@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 9, 2016

Updated

@dbu dbu merged commit 5ab2f5c into FriendsOfSymfony:master Jul 9, 2016
3 checks passed
3 checks passed
Scrutinizer 9 updated code elements
Details
StyleCI The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dbu added a commit that referenced this pull request Jul 9, 2016
@dbu

This comment has been minimized.

Copy link
Contributor

dbu commented Jul 9, 2016

thanks!
i noticed that documentation was missing, added a bit in 4a96c2c

do you now continue with a PR on the bundle? if you do, please also add documentation in the appropriate places to that people realize that this feature exists.

@gsdevme

This comment has been minimized.

Copy link
Contributor Author

gsdevme commented Jul 9, 2016

Thanks for accepting! Will do!

Sent from my iPhone

On 9 Jul 2016, at 10:51, David Buchmann notifications@github.com wrote:

thanks!
i noticed that documentation was missing, added a bit in 4a96c2c

do you now continue with a PR on the bundle? if you do, please also add documentation in the appropriate places to that people realize that this feature exists.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@gsdevme gsdevme deleted the gsdevme:feat/adds-a-noop-client branch Jul 21, 2016
@gsdevme gsdevme restored the gsdevme:feat/adds-a-noop-client branch Sep 29, 2016
@gsdevme gsdevme deleted the gsdevme:feat/adds-a-noop-client branch Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.