[Finder] Added a way to inverse a previous sorting #27967
Conversation
f4bbc64 to
b80853b
Compare
| * | ||
| * @return $this | ||
| */ | ||
| public function reverseSorting(bool $reverseSorting = true) |
There was a problem hiding this comment.
Just asking: why allow passing an argument here instead of doing this:
public function reverseSorting()
{
$this->reverseSorting = true;
return $this;
}There was a problem hiding this comment.
Because there is no way to reset a finder, so if you want to use it many time, but with different options, it's better to have a "real" setter
There was a problem hiding this comment.
but that's inconsistent with most other APIs, which cannot be reset either.
If you really want to have a bunch of common config for the Finder, and then some changes, the solution is to apply all the common config, and then clone the object before applying each variant
There was a problem hiding this comment.
Fair enough ; I fixed the code
b80853b to
36b151f
Compare
| { | ||
| public function __construct(iterable $iterator) | ||
| { | ||
| parent::__construct(array_reverse(iterator_to_array($iterator))); |
There was a problem hiding this comment.
iterable can be array, so you don't need iterator_to_array in that case.
There was a problem hiding this comment.
Well, the typehint should be Traversable then, to keep the code simple. The Finder component does not need support for arrays.
| { | ||
| public function __construct(iterable $iterator) | ||
| { | ||
| parent::__construct(array_reverse(iterator_to_array($iterator))); |
There was a problem hiding this comment.
The SortableIterator is using iterator_to_array($this->iterator, true). So should we do the same here ?
36b151f to
329bd50
Compare
|
@stof I addressed your comments. Thanks |
| @@ -460,6 +461,18 @@ public function sortByAccessedTime() | |||
| return $this; | |||
| } | |||
|
|
|||
| /** | |||
| * Reverse the sorting. | |||
329bd50 to
27bc298
Compare
| { | ||
| public function __construct(\Traversable $iterator) | ||
| { | ||
| parent::__construct(array_reverse(iterator_to_array($iterator, true))); |
There was a problem hiding this comment.
Instead of doing the reversing in the constructor, it should be done in a IteratorIterator::getIterator() method, like the SortableIterator does.
Which makes me wonder: should this concern be added to SortableIterator instead? Looks very related to me.
There was a problem hiding this comment.
You are right about the constructor vs getIterator. I have fixed the code.
About moving it to SortableIterator, I don't think it's a good idea
- If you don't sort, you can not reverse the sort. This is a bit weird, but if people want to reverse the "natural sorting" with this implemention, they could.
- Composition is a better design
Sometimes, it's useful to inverse the previous sorting. For exemple when you want to display the most recent uploaded files
27bc298 to
3cd0dca
Compare
we can, e.g. with a
not always: here, composition forces a double call to iterator_to_array + an extra call to array_reverse, while |
|
ping @lyrixx |
|
Thank you @lyrixx. |
…rixx) This PR was merged into the 4.2-dev branch. Discussion ---------- [Finder] Added a way to inverse a previous sorting | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | --- Sometimes, it's useful to inverse the previous sorting. For exemple when you want to display the most recent uploaded files Commits ------- 3cd0dca [Finder] Added a way to inverse a previous sorting
…s-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Finder] make reverse sorting a bit more generic | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27967 (comment) | License | MIT | Doc PR | - Commits ------- ce861d1 [Finder] make reverse sorting a bit more generic
…s-grekas) This PR was merged into the 4.2-dev branch. Discussion ---------- [Finder] make reverse sorting a bit more generic | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#27967 (comment) | License | MIT | Doc PR | - Commits ------- 340ede3 [Finder] make reverse sorting a bit more generic
Sometimes, it's useful to inverse the previous sorting.
For exemple when you want to display the most recent uploaded files