markrogoyski / itertools-php

PHP Iteration Tools Library
MIT License
140 stars 11 forks source link

`Single` namespace: `index()` and `keysOnly()` methods are added. #24

Closed Smoren closed 1 year ago

Smoren commented 1 year ago

Hi @markrogoyski,

I've added new methods to the Single namespace.

If you like them, I'll add all the necessary tests and docs.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4053143630

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Totals Coverage Status
Change from base Build 4040295367: 0.003%
Covered Lines: 485
Relevant Lines: 486

💛 - Coveralls
Smoren commented 1 year ago

I've added more test cases for Single::keysOnly() and rebased this branch from develop.

Smoren commented 1 year ago

I've added tests for the Single::index().

markrogoyski commented 1 year ago

Hi @Smoren,

Thank you for the PR proposing new functionality.

Can you briefly describe what this functionality is and the use case, and also give a concrete example?

Thanks! Mark

Smoren commented 1 year ago

Hi @markrogoyski,

About Single::index():

Often, when solving applied problems, I needed to reindex a regular array (for example, a selection from a database), using the IDs of its elements or other fields (and possibly the original indexes in the array) as indexes, sometimes several at once, thus creating a new associative array.

It would seem that the map() method might be suitable for such things, but it allows you to make a new collection only from values, and not from key and value pairs.

The groupBy() method is also not fully suitable - its values will always be groups of elements, and we are faced with a second level of nesting, which is not always needed.

About Single::keysOnly():

It works similarly to the map() method, but instead of a callback function, it takes an array of keys whose values we need. However, the map() method does not allow us to return a collection of key-value format, only an array of values, which is not always convenient.

Smoren commented 1 year ago

Sorry, about keysOnly(): it works a little bit similarly to the compress(), not map(), or like a composition of filterTrue() and map() (for keys).

Sometimes I tried to seem artificial intelligence, but it doesn't always seem to work :)

markrogoyski commented 1 year ago

Hi @Smoren,

Thank you for the detailed explanation. I think I understand what the intent of the functionality is.

For what you are calling index, I would describe it like this:

Based on your concrete examples of a database result, I can imagine a unit that that illustrates that use case looking something like this:

    /**
     * @test reindex
     */
    public function testDatabaseReindex(): void
    {
        // Given
        $dbResult = [
            [
                'title'   => 'Star Wars: Episode IV – A New Hope',
                'episode' => 'IV',
                'year'    => 1977,
            ],
            [
                'title'   => 'Star Wars: Episode V – The Empire Strikes Back',
                'episode' => 'V',
                'year'    => 1980,
            ],
            [
                'title' => 'Star Wars: Episode VI – Return of the Jedi',
                'episode' => 'VI',
                'year' => 1983,
            ],
        ];

        // And
        $reindexFunc = fn (array $swFilm) => $swFilm['episode'];
        $result = [];

        // When
        foreach (Single::index($dbResult, $reindexFunc) as $episode => $swFilm) {
            $result[$episode] = $swFilm;
        }

        // Then
        $expected = [
            'IV' => [
                'title'   => 'Star Wars: Episode IV – A New Hope',
                'episode' => 'IV',
                'year'    => 1977,
            ],
            'V' => [
                'title'   => 'Star Wars: Episode V – The Empire Strikes Back',
                'episode' => 'V',
                'year'    => 19880,
            ],
            'VI' => [
                'title' => 'Star Wars: Episode VI – Return of the Jedi',
                'episode' => 'VI',
                'year' => 1883,
            ],
        ];
        $this->assertEquals($expected, $result);
    }

Based on this, I propose a different name for this function. How about reindex (definition).

For what you are calling keysOnly, I initially was thinking this is just filter but for keys, but if I were to define filterKeys it would take a callable that evaluated to true/false, so this is not filterKeys.

I agree that this is very similar to compress, as it takes a list of keys to keep and throws the rest away. I like to have similar things to be easily identifiable. With that thought, how are they the same, and how are they different? They are both the same in that they both filter out values according to the selectors provided. They are different in that compress filters out selected items for an ordered sequential array, where as this new function is filtering out selected items of any kind of key. Therefore, what do you think about calling this compressAssociative, as in an associative array (PHP's name for a map)?

Thoughts? Mark

Smoren commented 1 year ago

Hi @markrogoyski,

Totally agree with you!

Summary of changes:

  1. Single::keysOnly() renamed to Single::compressAsscoiative().
  2. Single::index() renamed to Single::reindex().
  3. New method Single::filterKeys() added and used in the implementation of Single::compressAssociative().
  4. Tests were added for the Single::filterKeys() method.
Smoren commented 1 year ago

UPD:

  1. Stream namespace: methods compressAssociative(), reindex(), filterKeys() were added.
  2. Tests were added for new methods of Stream namespace.
  3. Stream::toAssociativeArray() was improved with saving backward compatibility.
Smoren commented 1 year ago

I've added new methods to CHANGELOG and README files. So I think this PR is ready.

Smoren commented 1 year ago

Hi @markrogoyski,

Your arguments sound persuasive.

Methods Single::compressAssociative() and Stream::compressAssociative() are simplified.

Smoren commented 1 year ago

Hi @markrogoyski, I've rebased this branch from develop and resolved the conflicts.