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.

Sign up for GitHub

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

Jump to bottom

Remove array_merge() from constructors when not needed #625

Closed
wants to merge 1 commit into from
Closed

Remove array_merge() from constructors when not needed #625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions src/core/etl/src/Flow/ETL/DSL/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

namespace Flow\ETL\DSL;

use Flow\ETL\Exception\InvalidArgumentException;
use Flow\ETL\Row;
use Flow\ETL\Row\EntryFactory;
use Flow\ETL\Row\EntryReference;
Expand Down Expand Up @@ -47,13 +46,7 @@ function optional(Expression $expression) : Expression

function struct(string ...$entries) : StructureReference
{
if (!\count($entries)) {
throw new InvalidArgumentException('struct (StructureReference) require at least one entry');
}

$entry = \array_shift($entries);

return new StructureReference($entry, ...$entries);
return new StructureReference(...$entries);
}

function lit(mixed $value) : Expression
Expand Down
9 changes: 7 additions & 2 deletions src/core/etl/src/Flow/ETL/Join/Comparison/All.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Flow\ETL\Join\Comparison;

use Flow\ETL\Exception\InvalidArgumentException;
use Flow\ETL\Join\Comparison;
use Flow\ETL\Row;
use Flow\ETL\Row\EntryReference;
Expand All @@ -18,9 +19,13 @@ final class All implements Comparison
*/
private array $comparisons;

public function __construct(Comparison $comparison, Comparison ...$comparisons)
public function __construct(Comparison ...$comparisons)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep it like it is now. This way, the constructor enforces at least one Comparison, so there is no need to write a unit test covering this edge case to satisfy mutation tests.
It's an approach commonly used in Scala when at least one element is required.
This particular array_merge should also have no impact on the pipeline processing performance.

{
$this->comparisons = \array_merge([$comparison], $comparisons);
if (!$comparisons) {
throw new InvalidArgumentException('All comparison requires at least one comparison');
}

$this->comparisons = $comparisons;
}

public function __serialize() : array
Expand Down
9 changes: 7 additions & 2 deletions src/core/etl/src/Flow/ETL/Join/Comparison/Any.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Flow\ETL\Join\Comparison;

use Flow\ETL\Exception\InvalidArgumentException;
use Flow\ETL\Join\Comparison;
use Flow\ETL\Row;
use Flow\ETL\Row\EntryReference;
Expand All @@ -18,9 +19,13 @@ final class Any implements Comparison
*/
private array $comparisons;

public function __construct(Comparison $comparison, Comparison ...$comparisons)
public function __construct(Comparison ...$comparisons)
{
$this->comparisons = \array_merge([$comparison], $comparisons);
if (!$comparisons) {
throw new InvalidArgumentException('Any comparison requires at least one comparison');
}

$this->comparisons = $comparisons;
}

public function __serialize() : array
Expand Down
10 changes: 8 additions & 2 deletions src/core/etl/src/Flow/ETL/Row/StructureReference.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

namespace Flow\ETL\Row;

use Flow\ETL\Exception\InvalidArgumentException;

/**
* @implements Reference<array{entries: array<string>, alias: ?string}>
*/
Expand All @@ -14,9 +16,13 @@ final class StructureReference implements Reference
/** @var array<string> */
private readonly array $entries;

public function __construct(string $entry, string ...$entries)
public function __construct(string ...$entries)
{
$this->entries = \array_merge([$entry], $entries);
if (!$entries) {
throw new InvalidArgumentException('StructureReference requires at least one entry');
}

$this->entries = $entries;
}

public function __serialize() : array
Expand Down
9 changes: 9 additions & 0 deletions src/core/etl/tests/Flow/ETL/Tests/Unit/Join/Comparison/AllTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,21 @@

use Flow\ETL\Adapter\Elasticsearch\Tests\Integration\TestCase;
use Flow\ETL\DSL\Entry;
use Flow\ETL\Exception\InvalidArgumentException;
use Flow\ETL\Join\Comparison;
use Flow\ETL\Join\Comparison\All;
use Flow\ETL\Row;

final class AllTest extends TestCase
{
public function test_empty_comparisons() : void
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('All comparison requires at least one comparison');

new All();
}

public function test_failure() : void
{
$comparison1 = $this->createStub(Comparison::class);
Expand Down
9 changes: 9 additions & 0 deletions src/core/etl/tests/Flow/ETL/Tests/Unit/Join/Comparison/AnyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,21 @@

use Flow\ETL\Adapter\Elasticsearch\Tests\Integration\TestCase;
use Flow\ETL\DSL\Entry;
use Flow\ETL\Exception\InvalidArgumentException;
use Flow\ETL\Join\Comparison;
use Flow\ETL\Join\Comparison\Any;
use Flow\ETL\Row;

final class AnyTest extends TestCase
{
public function test_empty_comparisons() : void
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Any comparison requires at least one comparison');

new Any();
}

public function test_failure() : void
{
$comparison1 = $this->createStub(Comparison::class);
Expand Down
Loading