HTTP message API #19
HTTP message API #19
Conversation
| $this->assertMessagesMatch($msg, $msg_copy); | ||
| } | ||
|
|
||
| public function testRequestMarshallingWithQueryParams() { |
There was a problem hiding this comment.
since this test method is almost identical to the one above it, a dataprovider could be used here instead. With just two, the copy/pasted code isn't that big a deal, but if you are going to add more similar tests, that would be better
| runs-on: macos-${{ matrix.version }} | ||
| strategy: | ||
| matrix: | ||
| version: [10.15] |
There was a problem hiding this comment.
Can we add 11? Usually MacOs is quickly adopted and 11 was around for half year or more already.
There was a problem hiding this comment.
No we cannot. See the commit where It took it out, because GitHub Actions fails it immediately, before even getting to our script.
There was a problem hiding this comment.
We can add it back when they fix it.
| @@ -1,5 +1,5 @@ | |||
| /* This is a generated file, edit the .stub.php file instead. | |||
There was a problem hiding this comment.
Should we version this one?
There was a problem hiding this comment.
Yes, because PHP5 can't generate it.
src/AWS/CRT/HTTP/Message.php
Outdated
| use AWS\CRT\Internal\Encoding; | ||
|
|
||
| abstract class Message extends NativeResource { | ||
| private $method = "GET"; |
There was a problem hiding this comment.
Why you setting it to GET here if it is overrode in the contructor?
Same for other private variables.
There was a problem hiding this comment.
I have a pathological hatred of uninitialized variables. In this case, it makes sense to just remove all the values, which I've done :(
* HTTP message API * Added MacOS CI * Grabbed fixes for mac from aws-crt-ffi
Includes fixes for OS X compilation and OS X CI.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.