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.

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

[Finder] Added a way to inverse a previous sorting #27967

Merged
merged 1 commit into from Oct 10, 2018

Conversation

@lyrixx
Copy link
Member

lyrixx commented Jul 16, 2018

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

@lyrixx lyrixx force-pushed the lyrixx:finder-reverse-iterator branch 2 times, most recently from f4bbc64 to b80853b Jul 16, 2018
*
* @return $this
*/
public function reverseSorting(bool $reverseSorting = true)

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jul 16, 2018

Member

Just asking: why allow passing an argument here instead of doing this:

public function reverseSorting()
{
    $this->reverseSorting = true;

    return $this;
}

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jul 16, 2018

Author Member

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

This comment has been minimized.

Copy link
@stof

stof Jul 16, 2018

Member

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

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jul 16, 2018

Author Member

Fair enough ; I fixed the code

@lyrixx lyrixx force-pushed the lyrixx:finder-reverse-iterator branch from b80853b to 36b151f Jul 16, 2018
{
public function __construct(iterable $iterator)
{
parent::__construct(array_reverse(iterator_to_array($iterator)));

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jul 17, 2018

Contributor

iterable can be array, so you don't need iterator_to_array in that case.

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2018

Member

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)));

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2018

Member

The SortableIterator is using iterator_to_array($this->iterator, true). So should we do the same here ?

@lyrixx lyrixx force-pushed the lyrixx:finder-reverse-iterator branch from 36b151f to 329bd50 Jul 17, 2018
@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Jul 17, 2018

@stof I addressed your comments. Thanks

@@ -460,6 +461,18 @@ public function sortByAccessedTime()
return $this;
}

/**
* Reverse the sorting.

This comment has been minimized.

Copy link
@fabpot

fabpot Jul 18, 2018

Member

reverses

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jul 19, 2018

Author Member

Thanks, Fixed

@lyrixx lyrixx force-pushed the lyrixx:finder-reverse-iterator branch from 329bd50 to 27bc298 Jul 19, 2018
@nicolas-grekas nicolas-grekas added this to the next milestone Jul 23, 2018
{
public function __construct(\Traversable $iterator)
{
parent::__construct(array_reverse(iterator_to_array($iterator, true)));

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 23, 2018

Member

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.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jul 23, 2018

Author Member

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

  1. 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.
  2. 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
@lyrixx lyrixx force-pushed the lyrixx:finder-reverse-iterator branch from 27bc298 to 3cd0dca Jul 23, 2018
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jul 23, 2018

If you don't sort, you can not reverse the sort

we can, e.g. with a SortableIterator::SORT_NONE = 0 const

Composition is a better design

not always: here, composition forces a double call to iterator_to_array + an extra call to array_reverse, while SortableIterator could handle that in one run by multiplying the return values of the comparator functions with 1/-1 depending on the sorting direction.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Sep 4, 2018

ping @lyrixx

@fabpot
fabpot approved these changes Oct 10, 2018
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Oct 10, 2018

Thank you @lyrixx.

@fabpot fabpot merged commit 3cd0dca into symfony:master Oct 10, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Oct 10, 2018
…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
fabpot added a commit that referenced this pull request Oct 10, 2018
…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
@lyrixx lyrixx deleted the lyrixx:finder-reverse-iterator branch Oct 11, 2018
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.