refactor: Update Transpose operation in point free style.
#178
Conversation
drupol
force-pushed
the
refactor/update-transpose-operation
branch
2 times, most recently
from
dc01e86
to
48f555c
Compare
drupol
force-pushed
the
refactor/update-transpose-operation
branch
from
48f555c
to
4798ce7
Compare
| return | ||
| $callbackForKeys = | ||
| /** | ||
| * @param Iterator<TKey, T> $iterator | ||
| * @param array $carry | ||
| * @param non-empty-array<int, TKey> $key | ||
| * | ||
| * @return Generator<TKey, list<T>> | ||
| * @return TKey | ||
| */ | ||
| static function (Iterator $iterator): Generator { | ||
| $mit = new MultipleIterator(MultipleIterator::MIT_NEED_ANY); |
There was a problem hiding this comment.
hmm removing this closure makes the operation impure now because of the MultipleIterator instantiation. Before this instantiation happened in this returned closure, which meant that the operation itself was still pure, even though it returned an impure closure.
Is there any way we can keep this behaviour in the refactor?
There was a problem hiding this comment.
Indeed. I asked the question on Psalm's slack channel and they told me to use a stub. I tried, but I'm going nowhere.
I wish I could find a proper way without altering the psalm baseline.
However, in this particular case I think we can safely ignore the issue.
- First, because the Transpose operation will never return a
MultipleIterator. - Second, because the Transpose operation is using a
MultipleIteratorobject to determine the result. This object is an intermediary variable(tool) that is used to find the resulting solution.
I will set this PR as draft in the meantime, I'll try to come up with a better solution.
There was a problem hiding this comment.
I quickly drafted a simpler example here: https://3v4l.org/LNuDT
Despite the fact that I use a MultipleIterator, this is completely pure.
There was a problem hiding this comment.
@drupol what I meant here is this: can we keep the same behaviour where we use the MultipleIterator in a Closure? I know it's not really an issue and it's safe to ignore, I'm just wondering if it can be done.
Something like:
public function __invoke(): Closure
{
return
static function () {
$pipe = ... // this might have to be a bit different
return $pipe; // might need to call it here e.g. $pipe();
}
}There was a problem hiding this comment.
Going to do a bit more research to fulfill your request and I'll let you know asap.
Pull request was converted to draft
Based on suggestion from @weirdan on Slack. > See https://psalm.dev/r/cc0354c625 and https://3v4l.org/LNuDT for simple examples. And Bruce's suggestion: > It'll still look pure to the outside: https://psalm.dev/r/6a4e6f8dde
drupol
force-pushed
the
refactor/update-transpose-operation
branch
from
e73270c
to
cd5e4d2
Compare
| @@ -26,46 +26,45 @@ final class Transpose extends AbstractOperation | |||
| /** | |||
| * @pure | |||
| * | |||
| * @psalm-suppress ImpureMethodCall | |||
There was a problem hiding this comment.
if we do end up keeping this we should always have clarifying comments for them, e.g.
@psalm-suppress ImpureMethodCall - using MultipleIterator as an internal tool which is not returned
There was a problem hiding this comment.
I just see this message! Good point, going to make the change.
This PR:
Transposein point free style.