PHP Guzzle generator #132 #140
Conversation
src/targets/php/guzzle6.js
Outdated
| } | ||
|
|
||
| module.exports.info = { | ||
| key: 'guzzle6', |
There was a problem hiding this comment.
It is not typical to specify a library version in the target name. Would remove, while you could keep it in the description. If we ever need to support other versions of guzzle, we can decide how best to support them, possibly via options if the syntax isn't too different
There was a problem hiding this comment.
Okay, I agree too!
I'll do it
|
If you are able, it would also be nice to install guzzle on our CI and add it to these files so that we can run the snippets in a real environment to ensure they are working (currently only doing this for a few targets): https://github.com/Kong/httpsnippet/blob/master/test/fixtures/cli.json#L14 |
|
@robertoarruda looking good! though the php clients array in |
|
|
|
@robertoarruda Hi! I saw @erunion approved this PR today and it drew my attention to it. Is this still something that you would like to get merged in? If so, would you mind resolving conflicts, and signing the above CLA and I can get it merged. Thanks! |
Sure! It's solved. |
erunion
left a comment
erunion
left a comment
There was a problem hiding this comment.
Super minor thing to clean up some of these snippets with no params but otherwise this still looks good!
|
|
||
| $client = new \GuzzleHttp\Client(); | ||
|
|
||
| $response = $client->request('PROPFIND', 'http://mockbin.com/har', [ |
There was a problem hiding this comment.
Can you update the target to only add the third param if it's got content? Should clean this example up a touch.
|
@robertoarruda Sorry, could you update the Guzzle target as well with this change? Tests are currently failing because they're generating snapshots that match the old style. |
7 participants
PHP Guzzle generator #132