[Finder] Case insensitive file sort #46126
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
4b9ad93 to
5b30852
Compare
| @@ -433,9 +433,14 @@ public function sort(\Closure $closure): static | |||
| * | |||
| * @see SortableIterator | |||
| */ | |||
| public function sortByName(bool $useNaturalSort = false): static | |||
| public function sortByName(bool $useNaturalSort = false, bool $useCaseInsensitive = false): static | |||
There was a problem hiding this comment.
A list of Boolean arguments is not an expressive method signature. Alternatively, I propose that we allow the sort flag to be passed as argument; but I'm not sure our backward compatibility policy allows to modify the type of the 1st argument from bool to bool|int.
Instead, we can add the method sortByNameCaseInsensitive(bool $useNaturalSort = false) (name to be discussed).
There was a problem hiding this comment.
Thank you for your comments.
I will work on a dedicated sort function and correct the changelog.
Is this method name good enough ? sortByNameCaseInsensitive(bool $useNaturalSort = false) It seems a bit long but it is self explanatory.
How about sortByCaseName(bool $useNaturalSort = false) which is closer to the php functions strcasecmp and strnatcasecmp ?
There was a problem hiding this comment.
3rd alternative, the Finder::sort() accepts a closure, but could also accept a value from a dedicated Enum.
@symfony/mergers I'm not sure if we can add a 2nd allowed type to an existing argument. From \Closure $arg to MyEnum|\Closure $arg
There was a problem hiding this comment.
Meanwhile I have moved the code to sortByCaseName(bool $useNaturalSort = false)
| public function sortByCaseName(bool $useNaturalSort = false): static | ||
| { | ||
| $this->sort = $useNaturalSort ? Iterator\SortableIterator::SORT_BY_NAME_NATURAL_CASE_INSENSITIVE : | ||
| Iterator\SortableIterator::SORT_BY_NAME_CASE_INSENSITIVE; |
| @@ -1,6 +1,11 @@ | |||
| CHANGELOG | |||
| ========= | |||
|
|
|||
| 6.1 | |||
There was a problem hiding this comment.
this will likely be 6.2 as we are past the feature freeze for 6.1
82d04f6 to
2f26995
Compare
|
I made the change to |
ee5661b to
7fdbb61
Compare
|
Thank you @hmoreau. |
|
Thank you @fabpot , @GromNaN , @javiereguiluz , @nicolas-grekas and @stof |
This PR was squashed before being merged into the 6.2 branch. Discussion ---------- [Finder] Add case insensitive sort Add a new sortByCaseInsensitiveName() method to have case insensitive sorting results. Like the sortByName(), pass true as its argument to use PHP's case insensitive natural sort order algorithm instead. Related to symfony/symfony#46126 PR Replace #16735 PR Commits ------- df9281f [Finder] Add case insensitive sort
Add a second argument $useCaseInsensitive to sortByName() method to select between case sensitive and case insensitive sort.