Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Finder] Added a way to inverse a previous sorting#27967
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f4bbc64 tob80853bCompare| * | ||
| * @return $this | ||
| */ | ||
| publicfunctionreverseSorting(bool$reverseSorting =true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just asking: why allow passing an argument here instead of doing this:
publicfunctionreverseSorting(){$this->reverseSorting =true;return$this;}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fair enough ; I fixed the code
b80853b to36b151fCompare| { | ||
| publicfunction__construct(iterable$iterator) | ||
| { | ||
| parent::__construct(array_reverse(iterator_to_array($iterator))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
iterable can be array, so you don't neediterator_to_array in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well, the typehint should beTraversable then, to keep the code simple. The Finder component does not need support for arrays.
| { | ||
| publicfunction__construct(iterable$iterator) | ||
| { | ||
| parent::__construct(array_reverse(iterator_to_array($iterator))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The SortableIterator is usingiterator_to_array($this->iterator, true). So should we do the same here ?
36b151f to329bd50Comparelyrixx commentedJul 17, 2018
@stof I addressed your comments. Thanks |
| } | ||
| /** | ||
| * Reverse the sorting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, Fixed
329bd50 to27bc298Compare| { | ||
| publicfunction__construct(\Traversable$iterator) | ||
| { | ||
| parent::__construct(array_reverse(iterator_to_array($iterator,true))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Instead of doing the reversing in the constructor, it should be done in aIteratorIterator::getIterator() method, like theSortableIterator does.
Which makes me wonder: should this concern be added toSortableIterator instead? Looks very related to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are right about the constructor vsgetIterator. 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 to3cd0dcaComparenicolas-grekas commentedJul 23, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
nicolas-grekas commentedSep 4, 2018
ping@lyrixx |
fabpot commentedOct 10, 2018
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 filesCommits-------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
Uh oh!
There was an error while loading.Please reload this page.
Sometimes, it's useful to inverse the previous sorting.
For exemple when you want to display the most recent uploaded files