Added QueryDefaultsPlugin #67
Conversation
This will fix #66
CHANGELOG.md
Outdated
| @@ -1,5 +1,10 @@ | |||
| # Change Log | |||
|
|
|||
| ## 1.5.0 - Unreleased | |||
There was a problem hiding this comment.
Hm, 1.5 or Unreleased? Using a version there is confusing, that's why we usually avoid it.
| { | ||
| public function it_is_initializable() | ||
| { | ||
| $this->beConstructedWith([]); |
There was a problem hiding this comment.
you can add this to a let method, it's like setUp in phpunit. If you use beConstructedWith again, it will override the on in the let method.
| public function it_is_initializable() | ||
| { | ||
| $this->beConstructedWith([]); | ||
| $this->shouldHaveType('Http\Client\Common\Plugin\QueryDefaultsPlugin'); |
There was a problem hiding this comment.
I think we use ::class constants wherever possible. phpspec 3.0 generates specs like that by default.
src/Plugin/QueryDefaultsPlugin.php
Outdated
| private $queryParams = []; | ||
|
|
||
| /** | ||
| * @param array $queryParams Hashmap of query name to query value |
There was a problem hiding this comment.
if i read the code correctly, keys and values should not be http encoded, right? i think we should mention that here to avoid confusion and double-encoded parameters
|
Thank you for your reviews. I have updated the PR accordingly. |
|
|
||
| public function it_is_a_plugin() | ||
| { | ||
| $this->shouldImplement(Plugin::class); |
There was a problem hiding this comment.
This will break php 5.4 compatibility. Even though I think we should drop 5.4 support, this is not a good enough reason.
FYI @sagikazarmark
src/Plugin/QueryDefaultsPlugin.php
Outdated
| private $queryParams = []; | ||
|
|
||
| /** | ||
| * @param array $queryParams Hashmap of query name to query value. Names and values should not be url encoded. |
There was a problem hiding this comment.
"should" is too weak. i would say "Names and values must not be url encoded as this plugin will encode them"
|
Thank you for your reviews. New tag is coming up. |
This will fix #66