Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plus operation RFC #313

Closed
rela589n opened this issue Nov 11, 2023 · 10 comments
Closed

Plus operation RFC #313

rela589n opened this issue Nov 11, 2023 · 10 comments
Labels

Comments

@rela589n
Copy link
Contributor

PHP plus operator

I'm wondering if there's same feature as php plus operator:

public function testPlusOperation(): void
{
    $initialArray =[
        'id' => ['A', 'B'],
        'id2' => ['C'],
    ];

    $resultArray = $initialArray + ['id' => [], 'id2' => [], 'id3' => []];

    self::assertSame([
        'id' => ['A', 'B'],
        'id2' => ['C'],
        'id3' => [],
    ], $resultArray);
}

As of me, personally I often use plus operation on arrays and it would make sense to implement it within a collection.

How it'd look like with collection

public function testCollectionPlusOperation(): void
{
    $initialCollection = Collection::fromIterable([
        ['id' => 'A'],
        ['id' => 'B'],
        ['id2' => 'C'],
    ]);

    $result = $initialCollection
        ->unwrap()
        ->groupBy(static fn ($value, $key) => $key)
        ->plus(['id' => [], 'id2' => [], 'id3' => []])
        ->all(false)
    ;

    self::assertSame([
        'id' => ['A', 'B'],
        'id2' => ['C'],
        'id3' => [],
    ], $result);
}

Comparison with prepend

With current implementation seemingly the same behavior may be achieved using prepend:

$result = $initialCollection
    ->unwrap()
    ->groupBy(static fn ($value, $key) => $key)
    ->prepend(...['id' => [], 'id2' => [], 'id3' => []])
    ->all(false)
;

self::assertSame([
    'id' => ['A', 'B'],
    'id2' => ['C'],
    'id3' => [],
], $result);

However it is not explicit solution, since the collection maintains all the keys until all() is called.

public function testPrependedKeysAreStillPresent(): void
{
    $initialCollection = Collection::fromIterable([
        ['id' => 'A'],
        ['id' => 'B'],
        ['id2' => 'C'],
    ]);

    $result = $initialCollection
        ->unwrap()
        ->groupBy(static fn ($value, $key) => $key)
        ->prepend(...['id' => [], 'id2' => [], 'id3' => []])
        ->pack()
        ->all()
    ;

    self::assertSame([
        ['id', []],
        ['id2', []],
        ['id3', []],
        ['id', ['A', 'B']],
        ['id2', ['C']],
    ], $result);
}

Therefore, when using prepend, we must convert collection into array before we can safely use the result, lest same instance iteration would yield id and id2 keys twice and id3 key before the rest.

Please, let me know what you think about this plus() proposal.

@drupol
Copy link
Collaborator

drupol commented Nov 11, 2023

Hi there!

Super cool idea !

Are you willing to submit a PR?

Thanks

@rela589n
Copy link
Contributor Author

Hi Pol, thanks for quick response!

Great to hear that!

Well, currently I have zero experience with functional programming and I doubt if this task is feasible for me. At least for now, this library looks like a black box with some magic inside. Not because something is wrong with it, - conversely it is me who is FP freshmen.

Thus, there will be no pull request from my side soon.

@drupol
Copy link
Collaborator

drupol commented Nov 11, 2023

OK I understand, no problem! I'll look into it when I have a bit of time.

Sponsoring is greatly appreciated though.

drupol added a commit that referenced this issue Nov 11, 2023
@drupol drupol mentioned this issue Nov 11, 2023
7 tasks
@drupol
Copy link
Collaborator

drupol commented Nov 11, 2023

Dear @rela589n,

I've implemented the feature in #314.

Would you be OK to test it?

Thanks!

@rela589n
Copy link
Contributor Author

It would be nice

Though, I'm not sure where to start from. I assume at first I should check documentation page https://loophp-collection.readthedocs.io/en/latest/pages/tests.html , but what should I do then? Maybe you could point out some similar test(s) examples I can inspire implementation from. And "Static analysis tests" - something really new to me.

Thank you

@drupol
Copy link
Collaborator

drupol commented Nov 11, 2023

You could simply checkout the branch containing the new operation and create a simple file in the root of the project test.php, containing:

<?php

declare(strict_types=1);

namespace Test;

use loophp\collection\Collection;

require_once __DIR__ . '/vendor/autoload.php';

$collection = Collection::fromIterable([1, 2, 3])
    ->plus([4, 5, 6])
    ->plus(['a' => 'a']);

foreach ($collection as $i => $r) {
    var_dump($r);
}

So that you can play with the new operation and see if it corresponds to what you were expecting.

drupol added a commit that referenced this issue Nov 11, 2023
drupol added a commit that referenced this issue Nov 11, 2023
@rela589n
Copy link
Contributor Author

I've checked it.

Works like a charm! It does exactly what is expected.

If you wish to add some unit tests, as a starting point you may use the following test case from my branch

feat/add-plus-operation...rela589n:loophp-collection:feat/add-plus-operation

Thank you!

@drupol
Copy link
Collaborator

drupol commented Nov 12, 2023

Thanks!
I'm using a specific workflow for tests based on data providers.
I made a new commit for tests, find it here: 9f29100

What would really help me is to create the documentation and the code example.

Copy link

Since this issue has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Nov 17, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2023
@drupol
Copy link
Collaborator

drupol commented Nov 22, 2023

@rela589n Are you willing to contribute to the documentation of the associated PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants