Skip to content
This repository was archived by the owner on Dec 6, 2022. It is now read-only.

Treat instances of DateTimeInterface as a value type throughout the API #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Treat instances of DateTimeInterface as a value type throughout the API #20

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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ dev
- Add `Providers\DSL\Compilation\Parameters\ParameterCollection::remove` to remove a previously added parameter.\
- Introduce `Analysis\INativeType::TYPE_NUMERIC` acting as a union type for `Analysis\INativeType::TYPE_INT` and `Analysis\INativeType::TYPE_DOUBLE`
- Fix bug when joining to the same `ITraversable` instance with an `->indexBy(...)` only returning the first element.
- Change identity hash to hash instances of `DateTimeInterface` by class and timestamp hence treating it as a value type throughout the PINQ API
Copy link

@Ocramius Ocramius Apr 13, 2015

Choose a reason for hiding this comment

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

As discussed below, I think that building exclusions on a case-by-case basis is a problem. This sort of feature may need some better thought work on the concept of Value Objects, I'd say (and providing a way to extract scalar values/identity from them, eventually)

Copy link
Owner Author

@TimeToogo TimeToogo Apr 13, 2015

Choose a reason for hiding this comment

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

@Ocramius Hmm.. this is a tough one, perhaps some sort of static registrar defining hasher implementations for a hasher interface for a specific class? The main reason I am considering DateTime as a special case is for compatibility with SQL where date times are a value type. For instance:

$results = $table
        ->groupBy(function ($row) { return $row['some_date']; })
        ->select(function ($group, \DateTime $date) {
               return [
                    'date'  => $date,
                    'count' => $group->count()
              ];
        });

Where $row['some_date'] is a datetime in the database represented as an instance of DateTime in the query, this will not map well to SQL as it stands as in fact each row containing an instance of DateTime will produce a new group. This update fixes that as the behaviour of the API now matches SQL.

As you have mentioned, perhaps it is worthwhile implementing way for users to customize this behaviour on a per-class approach. The trouble I have with this idea is again the compatibility with SQL, when mapping entities to rows this is all well and good because what is the instance in PHP corresponds to the primary key in SQL. For value objects, this is more difficult as there is no language level concept in PHP, if we define a way for users to register a special hasher for their classes, it will have to be in some formalized manner that can be interpreted by a query provider. I don't believe it would be a good idea just to provide an interface as such:

interface IClassIdentifier
{
    public function getClassType(): string;
    public function getIdentityString($instance): string;
}

There would be no way for any query provider to interpret such an implementation without inspecting the implementation of getIdentityString method which is just nasty. The reason I bring all this this up is because I have begun implementing a query provide like Pinq.Demo.Sql but a much more thorough implementation using the Doctrine so this is at the forefront of my mind. Will have to think this over some more, thoughts?

Copy link

@Ocramius Ocramius Apr 13, 2015

Choose a reason for hiding this comment

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

@TimeToogo the problem is that you are taking an opinionated approach on something that has different opinions all across the interwebz.

We decided to go with a by-ref check to avoid the problem overall until we have a way to solve it.

DBAL types are a semi-working solution (though static and opinionated).

In general, Value Objects would be the way to go, with a way to serialize/unserialize for in-memory and in-sql comparison.

Copy link

@Ocramius Ocramius Apr 13, 2015

Choose a reason for hiding this comment

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

So yeah, serializing would be context-aware :|

Copy link
Owner Author

@TimeToogo TimeToogo Apr 13, 2015

Choose a reason for hiding this comment

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

@Ocramius Yep makes sense, like above an unopinionated approach would be the IClassIdentifier (user provides implementations for their classes) that would modify how comparisons are made between classes as the user chooses. The problem is that then the API and how it behaves is a moving target, to the whim of the user. Query providers (other implementations of the API) would most likely am at the largest possible support for the API but this becomes harder as these core behaviours are no longer well-defined.

So really it comes down to priorities and the formalization of behaviour. Define a set standards of value of objects such as DateTime (opinionated) or let the user define such behaviour and instead query providers who implement the API now behave different due to the customizations from the user. As usual both sides have there pros and cons :/


3.1.0 (29/3/15)
===============
Expand Down
4 changes: 3 additions & 1 deletion Source/Iterators/Common/Identity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ public static function hash($value)

case 'o': //object

return 'o' . spl_object_hash($value);
return $value instanceof \DateTimeInterface || $value instanceof \DateTime
? 'dt:' . get_class($value) . ':' . $value->getTimestamp()
Copy link

@Ocramius Ocramius Apr 13, 2015

Choose a reason for hiding this comment

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

Is dt: a magic constant?

Copy link

@Ocramius Ocramius Apr 13, 2015

Choose a reason for hiding this comment

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

What happens for other value types? For instance, we kept rejecting this particular sort of feature throughout doctrine because there is no way to build a "special cases" map.

While I agree that it makes developer experience better, it also causes a series of #WTF moments.

What if you could simply have the concept of "value objects" and a map of which ones are VOs? Acceptable? Better? Worse?

Copy link
Owner Author

@TimeToogo TimeToogo Apr 13, 2015

Choose a reason for hiding this comment

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

@Ocramius The identity::hash function returns a unique string representing if two values are identical (===), to avoid collisions each type of value has a prefix, dt is just an arbitrary prefix string I choose for date times. This is used throughout the implementation of the API to compare values.

: 'o' . spl_object_hash($value);

case 'a': //array

Expand Down
12 changes: 12 additions & 0 deletions Tests/Helpers/ExtendedDateTime.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

namespace Pinq\Tests\Helpers;

/**
*
* @author Elliot Levin <[email protected]>
*/
class ExtendedDateTime extends \DateTime
{

}
30 changes: 30 additions & 0 deletions Tests/Integration/Collection/GroupJoinApplyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,36 @@ public function testThatApplyGroupJoinWithDefaultValueOperatedCorrectly(\Pinq\IC
'10:1,3,5,7,9,11,13,15,17,19',
]);
}

/**
* @dataProvider oneToTen
*/
public function testThatOnEqualityWillNotMatchNullsAndUseDefault(\Pinq\ICollection $collection, array $data)
{
$collection
->groupJoin($collection)
->onEquality(
function ($i) { return $i % 2 === 0 ? $i : null; },
function ($i) { return $i % 2 === 0 ? $i : null; })
->withDefault('<DEFAULT>')
->apply(function (&$outer, \Pinq\ITraversable $innerGroup) {
$outer .= ':' . $innerGroup->implode('-');
});

$this->assertMatches($collection, [
'1:<DEFAULT>',
'2:2',
'3:<DEFAULT>',
'4:4',
'5:<DEFAULT>',
'6:6',
'7:<DEFAULT>',
'8:8',
'9:<DEFAULT>',
'10:10'
]);
}

/**
* @dataProvider oneToTen
*/
Expand Down
34 changes: 14 additions & 20 deletions Tests/Integration/Collection/JoinApplyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,31 +206,25 @@ function ($i) { return $i % 2 === 0 ? $i : null; })
}

/**
* @dataProvider oneToTen
* @dataProvider emptyData
*/
public function testThatOnEqualityWillNotMatchNullsAndUseDefault(\Pinq\ICollection $collection, array $data)
public function testThatOnEqualityWillPassForValueWiseIdenticalDateTimes(\Pinq\ICollection $collection, array $data)
{
$dateTime = new \DateTime('2-1-2001');
$anotherDateTime = new \DateTime('2-1-2000');
$collection[] = $dateTime;
$collection[] = $anotherDateTime;

$collection
->groupJoin($collection)
->onEquality(
function ($i) { return $i % 2 === 0 ? $i : null; },
function ($i) { return $i % 2 === 0 ? $i : null; })
->withDefault('<DEFAULT>')
->apply(function (&$outer, \Pinq\ITraversable $innerGroup) {
$outer .= ':' . $innerGroup->implode('-');
->join([$dateTime])
->onEquality(function ($outer) { return $outer; }, function ($inner) { return $inner; })
->apply(function (&$outer) {
$outer = '<MATCHED>';
});

$this->assertMatches($collection, [
'1:<DEFAULT>',
'2:2',
'3:<DEFAULT>',
'4:4',
'5:<DEFAULT>',
'6:6',
'7:<DEFAULT>',
'8:8',
'9:<DEFAULT>',
'10:10'
$this->assertMatchesValues($collection, [
'<MATCHED>',
$anotherDateTime
]);
}
}
16 changes: 16 additions & 0 deletions Tests/Integration/Collection/RemoveTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,20 @@ public function testThatRemoveWillRemovesIdenticalValuesFromCollectionAndPreserv

$this->assertMatchesValues($collection, $data);
}

/**
* @dataProvider dateTimes
*/
public function testRemovesDateTimeClassesByValueRatherThenReference(
\Pinq\ICollection $collection,
array $data
) {
$originalCount = $collection->count();
$dateTime = $collection[2];

$collection->remove($dateTime);

$this->assertCount($originalCount - 1, $collection);
$this->assertNotContains($dateTime, $collection);
}
}
19 changes: 18 additions & 1 deletion Tests/Integration/Collection/SetIndexTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function testThatSettingAnIndexWillOverrideTheElementInTheCollection(\Pin
}

/**
* @dataProvider everything
* @dataProvider emptyData
*/
public function testThatSetIndexWithNoKeyAppendsTheValueWithTheNextLargestIntGreaterThanOrEqualToZeroLikeAnArray(\Pinq\ICollection $collection, array $data)
{
Expand Down Expand Up @@ -53,4 +53,21 @@ public function testThatSetIndexWithNoKeyAppendsTheValueWithTheNextLargestIntGre

$this->assertSame('boo', $collection[1]);
}

/**
* @dataProvider emptyData
*/
public function testThatDateTimeIndexesAreComparedByValue(\Pinq\ICollection $collection, array $data)
{
$dateTime = new \DateTime('2-1-2001');
$anotherDateTime = new \DateTime('2-1-2000');

$collection[$dateTime] = 1;
$collection[$anotherDateTime] = 2;
$collection[clone $anotherDateTime] = 3;

$this->assertSame(1, $collection[$dateTime]);
$this->assertSame(3, $collection[$anotherDateTime]);
$this->assertSame(3, $collection[clone $anotherDateTime]);
}
}
18 changes: 18 additions & 0 deletions Tests/Integration/Collection/UnsetIndexTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,22 @@ public function testThatUnsetingAnIndexWillRemoveTheElementFromTheCollection(\Pi

$this->assertMatches($collection, $data);
}

/**
* @dataProvider emptyData
*/
public function testThatDateTimeIndexesAreComparedByValue(\Pinq\ICollection $collection, array $data)
{
$dateTime = new \DateTime('2-1-2001');
$anotherDateTime = new \DateTime('2-1-2000');

$collection[$dateTime] = 1;
$collection[$anotherDateTime] = 2;
unset($collection[new \DateTime('2-1-2000')]);

$this->assertTrue(isset($collection[$dateTime]));
$this->assertFalse(isset($collection[$anotherDateTime]));
$this->assertFalse(isset($collection[clone $anotherDateTime]));
$this->assertFalse(isset($collection[new \DateTime('2-1-2000')]));
}
}
14 changes: 13 additions & 1 deletion Tests/Integration/DataTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ public function everything()
'oneToTenTwice',
'assocOneToTen',
'tenRandomStrings',
'assocMixedValues'
'assocMixedValues',
'dateTimes'
];

foreach ($dataProviders as $provider) {
Expand Down Expand Up @@ -106,6 +107,17 @@ public function assocTenRandomStrings()
return $this->getImplementations(array_combine($this->randomStrings(10), $this->randomStrings(10)));
}

public function dateTimes()
{
return $this->getImplementations(iterator_to_array(
new \DatePeriod(
Copy link

@Ocramius Ocramius Apr 13, 2015

Choose a reason for hiding this comment

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

Some alignment weirdness here (minor)

new \DateTime('1-1-2000'),
new \DateInterval('P4M'),
new \DateTime('2-3-2004')
)
));
}

private function randomStrings($amount)
{
$letters = 'qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM1234567890-!@#$%^&*()_';
Expand Down
22 changes: 22 additions & 0 deletions Tests/Integration/Traversable/GetIndexTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,26 @@ public function testThatIndexesSupportArrayKeys(\Pinq\ITraversable $traversable,
'Should be using strict equality for arrays (order matters)');
}
}

/**
* @dataProvider emptyData
*/
public function testThatDateTimeIndexesAreComparedByValueAndClass(\Pinq\ITraversable $traversable, array $data)
{
$dateTime = new \DateTime('2-1-2001');
$anotherDateTime = new \DateTime('2-1-2000');

$traversable = $traversable
->append([$dateTime, $anotherDateTime])
->indexBy(function ($value) { return $value; })
->select(function (\DateTime $dateTime) { return $dateTime->format('j-n-Y'); });

$this->assertSame('2-1-2001', $traversable[$dateTime]);
$this->assertSame('2-1-2001', $traversable[clone $dateTime]);
$this->assertSame('2-1-2001', $traversable[new \DateTime('2-1-2001')]);

if(class_exists('DateTimeImmutable', false)) {
$this->assertNull($traversable[new \DateTimeImmutable('2-1-2001')]);
}
}
}
21 changes: 21 additions & 0 deletions Tests/Integration/Traversable/GroupByTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Pinq\Tests\Integration\Traversable;

use Pinq\ITraversable;

class GroupByTest extends TraversableTest
{
protected function _testReturnsNewInstanceOfSameTypeWithSameScheme(\Pinq\ITraversable $traversable)
Expand Down Expand Up @@ -137,4 +139,23 @@ public function testThatGroupByMaintainsArrayReferences(\Pinq\ITraversable $trav

$this->assertSame($data, [[1], [2], [1, 2, 'foo' => 1], [3, 5, 'foo' => 3], [4, 2, 'foo' => 4]]);
}

/**
* @dataProvider emptyData
*/
public function testThatDateTimeAreGroupedByValue(\Pinq\ITraversable $traversable, array $data)
{
$traversable = $traversable
->append([2000, 2000, 2001, 2000, 2002, 2000, 2001, 2000, 2002])
->groupBy(function ($year) { return new \DateTime('1-1-' . $year); })
->select(function (ITraversable $years, \DateTime $dateTime) {
return [$dateTime->format('Y'), $years->count()];
});

$this->assertMatchesValues($traversable, [
['2000', 5],
['2001', 2],
['2002', 2],
]);
}
}
20 changes: 20 additions & 0 deletions Tests/Integration/Traversable/IssetIndexTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Pinq\Tests\Integration\Traversable;

use Pinq\Tests\Helpers\ExtendedDateTime;

class IssetIndexTest extends TraversableTest
{
/**
Expand All @@ -27,4 +29,22 @@ public function testThatIssetOnInvalidIndexesReturnFalse(\Pinq\ITraversable $tra

$this->assertFalse(isset($traversable[$notAnIndex]));
}

/**
* @dataProvider emptyData
*/
public function testThatDateTimeIndexesAreComparedByValueAndClass(\Pinq\ITraversable $traversable, array $data)
{
$dateTime = new \DateTime('2-1-2001');
$anotherDateTime = new \DateTime('2-1-2000');

$traversable = $traversable
->append([$dateTime, $anotherDateTime])
->indexBy(function ($value) { return $value; });

$this->assertTrue(isset($traversable[$dateTime]));
$this->assertTrue(isset($traversable[clone $dateTime]));
$this->assertTrue(isset($traversable[new \DateTime('2-1-2001')]));
$this->assertFalse(isset($traversable[new ExtendedDateTime('2-1-2001')]));
}
}
21 changes: 21 additions & 0 deletions Tests/Integration/Traversable/JoinTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,4 +374,25 @@ public function testJoinToSelfWithInnerIndexBy(\Pinq\ITraversable $traversable)
'4:3',
]);
}

/**
* @dataProvider emptyData
*/
public function testThatOnEqualityWillPassForValueWiseIdenticalDateTimes(\Pinq\ITraversable $traversable, array $data)
{
$dateTime = new \DateTime('2-1-2001');
$anotherDateTime = new \DateTime('2-1-2000');

$traversable = $traversable
->append([$dateTime, $anotherDateTime])
->join([$dateTime])
->onEquality(function ($outer) { return $outer; }, function ($inner) { return $inner; })
->to(function (\DateTime $outer) {
return $outer->format('d-m-Y');
});

$this->assertMatchesValues($traversable, [
'02-01-2001',
]);
}
}
38 changes: 38 additions & 0 deletions Tests/Integration/Traversable/UnionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Pinq\Tests\Integration\Traversable;

use Pinq\Tests\Helpers\ExtendedDateTime;

class UnionTest extends TraversableTest
{
protected function _testReturnsNewInstanceOfSameTypeWithSameScheme(\Pinq\ITraversable $traversable)
Expand Down Expand Up @@ -63,4 +65,40 @@ public function testThatUnionMaintainsReferences(\Pinq\ITraversable $traversable

$this->assertSame('a-b-c-d-e-f-', implode('', $data));
}

/**
* @dataProvider emptyData
*/
public function testThatUnionWillMatchDateTimeByTimestampAndClass(\Pinq\ITraversable $traversable, array $data)
{
$traversable = $traversable
->append([
new \DateTime('2-1-2001'),
new \DateTime('2-1-2001'),
new \DateTime('2-1-2001 00:00:01'),
new \DateTime('1-1-2001'),
new \DateTime('1-1-2001'),
new \DateTime('3-1-2001'),
new ExtendedDateTime('1-1-2001'),
])
->union([
new \DateTime('4-1-2001'),
new \DateTime('2-1-2001'),
new ExtendedDateTime('1-1-2001'),
new ExtendedDateTime('2-1-2001'),
])
->select(function (\DateTime $outer) {
return get_class($outer) . ':' . $outer->format('d-m-Y H:i:s');
});

$this->assertMatchesValues($traversable, [
'DateTime:02-01-2001 00:00:00',
'DateTime:02-01-2001 00:00:01',
'DateTime:01-01-2001 00:00:00',
'DateTime:03-01-2001 00:00:00',
'Pinq\Tests\Helpers\ExtendedDateTime:01-01-2001 00:00:00',
'DateTime:04-01-2001 00:00:00',
'Pinq\Tests\Helpers\ExtendedDateTime:02-01-2001 00:00:00',
]);
}
}
Loading