Http headers #3
Http headers #3
Conversation
src/Json.php
Outdated
| @@ -57,27 +57,27 @@ public function getEndpoint() | |||
| return $this->endpoint; | |||
| } | |||
|
|
|||
| public function get($path, array $params = []) | |||
| public function get($path, array $params = [], array $headers = []) | |||
There was a problem hiding this comment.
What do you think of renaming $headers to $options, and make 'headers' a optional key in options? I think it could make it more resilient to changes in the future, if we need any. As in:
$client->get('/foo', [], [ 'headers' => ['foo' => 'bar'] ]);There was a problem hiding this comment.
I'm not sure how many other options are typically passed and if we're not making people type more than necessary, but sure! I don't object. I'll fiddle with it.
There was a problem hiding this comment.
Is this better? :)
There was a problem hiding this comment.
FYI, I noticed that I'd forgotten to fix the interface variable names (and their description), so I tweaked that with one of the latest commits.
tests/JsonTest.php
Outdated
| */ | ||
| public function passesOnHeaders() | ||
| { | ||
| $this->mockClient()->request('GET', $this->url('/foo'), ['headers' => ['Content-Type' => 'application/json; charset=utf-8'], "query" => []]) |
There was a problem hiding this comment.
The above line is a bit too long and hard to read, IMO.
What do you think of...?
$options = [
'headers' => ['Content-Type' => 'application/json; charset=utf-8'],
'query' => []
];
$this->mockClient()->request('GET', $this->url('/foo'), $options)| @@ -85,8 +85,8 @@ Only tested code will be accepted. Please follow fix the style guide. | |||
|
|
|||
There was a problem hiding this comment.
Can you please run those for some code standard fixes?
./vendor/bin/broc-code fix src
./vendor/bin/broc-code fix tests
There was a problem hiding this comment.
I did actually! (Well, bro-code, there was no broc-code - that's why I changed it in the README.md.) It didn't seem to have changed anything.
There was a problem hiding this comment.
Oh - can confirm that it's working despite my scepticism, I put in your $options snippet above and it changed the indentation of the arrow slightly. :D
|
Thank you for your contribution! =) |
|
Fixes #2 |
|
One thing I forgot. README says nothing about the new feature =) |
|
Ah! See, that's the sort of thing I didn't realise was my prerogative in a pull request. I'll fiddle something. |
Hi, Marcelo!
Here's a pull request for the header topic. I wasn't entirely sure about the
array_filter()that I put in - I took the larger number of failing tests without it as an indicator that adding it in might be truer to your intentions. (I didn't want to start putting in anif (!empty($options['headers']))or similar highly specific code.)Let me know if this is acceptable or if you'd like tweaks.