Update all operation to prevent data loss
#209
Conversation
- apply `normalize` by default - allow usage without normalize via bool param
|
@drupol hey, let me know what you think of the comments above and the implementation; if all is good I will continue to complete the outstanding checkboxes |
|
It seems to go in the proper direction, nothing to add here, all good. |
| If you want to ensure no data is lost in the case of duplicate keys, look at the ``Collection::normalize()`` operation. | ||
| .. warning:: An earlier version of this operation did not re-index keys by default, which meant at times data loss could occur. | ||
| The reason data loss could occur is because PHP array keys cannot be duplicated and must either be ``int`` or ``string``. | ||
| The old behaviour can still be achieved by using the operation with the ``$normalize`` parameter as *false*. |
There was a problem hiding this comment.
I think we should remove this parameter in the next major version and keep this behavior, WDYT ?
To restore the previous behavior, users can do: iterator_to_array($collection->getIterator());
There was a problem hiding this comment.
it's a bit trickier because of this: #208 (comment). Also see the serialization comment below
| $piped = Pipe::of()(Filter::of()($even), All::of())(new ArrayIterator([1, 2, 3, 4])); | ||
| print_r($piped); // [2, 4] |
There was a problem hiding this comment.
this standalone usage is actually not valid. In order to use Pipe every operation needs to return an Iterator, but All returns an array.
This was missed when introducing the All operation because there isn't any enforcement of the return type at the Operation interface level. It's not easy to implement that however because you could return multiple closures and only the last one needs to return an Iterator.
I will look to make a separate PR after this to update the All operation to return a Generator though, and move the conversion to array back into the Collection method
There was a problem hiding this comment.
Indeed, let's fix that in the next PR.
| @@ -624,7 +624,7 @@ public function isEmpty(): bool | |||
| */ | |||
| public function jsonSerialize(): array | |||
| { | |||
| return $this->all(); | |||
| return $this->all(false); | |||
There was a problem hiding this comment.
we cannot normalize by default in jsonSerialize because that would break usage with a "map" collection, e.g.:
// Correct -> using `all(false)`
json_encode(Collection::fromIterable(['foo' => 1, 'bar' => 2])); // '{"foo": 1, "bar": 2}'
// Incorrect -> using the new `all()`
json_encode(Collection::fromIterable(['foo' => 1, 'bar' => 2])); // '[1, 2]'@drupol also I realised there's no mention in the documentation that Collection can be serialized. I am thinking to write about it and mention this behaviour as well. What do you think is the best place for it? I would say we could mention it in 2 places:
usage.rst-> have a short "Serialization" sectionapi.rst-> mentioning thejsonSerializemethod
There was a problem hiding this comment.
Oh! Nice catch.
I would do it in both files - as serialization is a feature AND part of the API.
|
@drupol hey, I'd say this is ready for final review. Let me know if there's anything else we should add/modify in here |
|
Youhou! Thanks! |
Thank you as well! |
This PR:
alloperation to applynormalizeby default. Also allows usage withoutnormalizevia a bool paramFixes #208.