Before first release

[Nyholm]

Before we release 0.1.0 of this package we should review this package to make sure it is properly tested and complies with LSP.

Also ping @fbourigault.

[kelunik]

This needs investigation for the currently skipped tests, where Artax rightfully sends no Content-Length header for TRACE requests, as a body is disallowed for TRACE requests.

See https://tools.ietf.org/html/rfc7231#section-4.3.8.

[kelunik]

@Nyholm Should be fine after #4 passes and is merged, right?

[Nyholm]

I think so. I let someone else also give their thumbs up. Maybe @joelwurtz wants to have a ping here as well.

[dbu]

i had a look at the code. some remarks:

• artax-adapter/src/Client.php

  Line 33 in 69ef4c7

  /** @var Artax\Request $req */

do you have to use your own request implementation? could you do an instanceof check to see if its already used? would it make sense to provide a request factory for your implementation? or ar artax request not psr-7 compatible?

why annotate this? its clear from the code what class this will be. and on the next lines, why not chain the calls?

• artax-adapter/src/Client.php

  Line 42 in 69ef4c7

  Artax\Client::OP_MAX_REDIRECTS => 0,

can this not be configured when instantiating the client? that would be more consistent with how we handle guzzle. if no client is specified, we create it without following redirects. but if the user gives us a guzzle instance we respect its configuration and don't overwrite it on a per-request basis.

• artax-adapter/src/Client.php

  Lines 48 to 57 in 69ef4c7

  	$respBody = $resp->getBody();
  	$bodyStream = $this->streamFactory->createStream();
  	while (null !== $chunk = yield $respBody->read()) {
  	$bodyStream->write($chunk);
  	}
  	$bodyStream->rewind();
  	return $this->responseFactory->createResponse(

this could be quite a problem with large files. if the artax response was psr compatible, we should simply return that instead to avoid shoveling the whole stream into a new stream.

• class names

i think elsewhere we did things like use Amp\Artax\Request as ArtaxRequest; rather than reference classes with partial namespaces. its a matter of taste, but wanted to raise the question.

[kelunik]

do you have to use your own request implementation? could you do an instanceof check to see if its already used? would it make sense to provide a request factory for your implementation? or ar artax request not psr-7 compatible?

Yes, because Artax isn't PSR-7 compatible. It can't be, because PSR-7 isn't compatible with async.

why annotate this? its clear from the code what class this will be. and on the next lines, why not chain the calls?

Because PhpStorm is currently buggy and loses the type information somewhere, but chaining is indeed possible.

can this not be configured when instantiating the client? that would be more consistent with how we handle guzzle. if no client is specified, we create it without following redirects. but if the user gives us a guzzle instance we respect its configuration and don't overwrite it on a per-request basis.

Yes, it can be configured for the default client as well, I thought it's better when it's always the same behavior, but if Guzzle already does it the other way, I'm fine with that, too.

this could be quite a problem with large files. if the artax response was psr compatible, we should simply return that instead to avoid shoveling the whole stream into a new stream.

If PSR-7 was async compatible. ;-) I think this can be changed, let me have a look.

class names

Don't care, it's just easier to have one import statement than aliasing every class.

[joelwurtz]

I'm fine with the current implementation, my biggest concern consist of having the full stream in mémory like @dbu said.

But this is something that can be changed without being BC. IMO We should not try to do too many optimization before launching it as the long term will certainly differ from what we think. That's why we have 0.X versions anyway.

It's compliant with httplug, tests works for me 👍

By the way for the stream, i think you can maybe, instead of copying, provide your own psr7 stream implementation that encapsulate the artax stream and deliver data as the client need.

Didn't see the PR forget this :)

[kelunik]

@dbu @Nyholm @joelwurtz We're ready to go now, right? Do you want to tag a release or should I do that?

[Nyholm]

Please go ahead and tag 0.1.0.

[kelunik]

Done. 👍