Feature: "relative" stream #10
Conversation
| } | ||
|
|
||
| /** | ||
| * @inheritdoc |
|
@samsonasik thanks for spotting - updated. |
| * Class constructor | ||
| * | ||
| * @param StreamInterface $decodatedStream | ||
| * @param $offset |
There was a problem hiding this comment.
@param paramType $offset
mtymek
force-pushed
the
feature/relative_stream
branch
from
a5a4895 to
116280b
Compare
| public function __construct(StreamInterface $decodatedStream, $offset) | ||
| { | ||
| $this->decoratedStream = $decodatedStream; | ||
| $this->offset = $offset; |
There was a problem hiding this comment.
cast to int or check that it is an unsigned integer
| * @param StreamInterface $decoratedStream | ||
| * @param int $offset | ||
| */ | ||
| public function __construct(StreamInterface $decoratedStream, $offset) |
There was a problem hiding this comment.
Would it make sense to also provide $limit = null? Or maybe am I being a feature creep here? /cc @weierophinney
There was a problem hiding this comment.
I would leave it until it is needed - can be added any time.
There was a problem hiding this comment.
makes sense. We can build separate stream classes for that use-case if it ever comes up.
|
Added test. |
|
Looking good. Can anyone else proof-read it before I merge? |
|
👍 IMO |
|
Pushed CS fix to prevent build from failing. |
| { | ||
| public function testToString() | ||
| { | ||
| $decorated = $this->prophesize(Stream::class); |
There was a problem hiding this comment.
This is the first use I've seen of Prophecy. Guess I have a bit of studying to do in the near future!
There was a problem hiding this comment.
Once you start using it, you will never want to go back :-) Start here: https://thephp.cc/news/2015/02/phpunit-4-5-and-prophecy
There was a problem hiding this comment.
I'll actually say that I don't like neither mockery nor prophecy mainly due to the lack of autocompletion. Yes, feel free to throw stuff at me now :-P
There was a problem hiding this comment.
Sometimes I "cheat" in my tests to get autocompletion:
/** @var ObjectProphecy|Stream $decorated */
$decorated = $this->prophesize(Stream::class);
$decorated->seek(100, SEEK_SET)->shouldBeCalled();
$decorated->getContents()->shouldBeCalled()->willReturn('foobarbaz');There was a problem hiding this comment.
I hate those comments. :)
|
👍 |
| * It can be used to avoid copying full stream, conserving memory. | ||
| * @example see Zend\Diactoros\AbstractSerializer::splitStream() | ||
| */ | ||
| class RelativeStream implements StreamInterface |
There was a problem hiding this comment.
@weierophinney what do you think about final here? No methods defined besides the interface.
There was a problem hiding this comment.
I think marking it final makes sense.
Initial implementation of "relative" stream, representing subpart of another stream, to be reviewed.
It is a first step towards parsing large bodies that may not fit in PHP's available memory.
There are at least two concerns for now:
tell()andseek(), but it would also complicate things a lot (need to check if stream is seekable, etc). Should I care it at all?Note that test suite is not finished - I will do it during the weekend.