NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Finder] Case insensitive file sort#46126

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:6.2fromhmoreau:CASE_INSENSITIVE_FILE_LIST

Jul 22, 2022

Merged

[Finder] Case insensitive file sort#46126

fabpot merged 1 commit intosymfony:6.2fromhmoreau:CASE_INSENSITIVE_FILE_LIST

Jul 22, 2022

Conversation

Copy link

Contributor

hmoreau commentedApr 20, 2022•
edited
Loading

Q	A
Branch?	6.2
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR	symfony/symfony-docs#16735

Add a second argument$useCaseInsensitive to sortByName() method to select between case sensitive and case insensitive sort.

carsonbot added Status: Needs Review Feature Finder labels

Apr 20, 2022

carsonbot added this to the6.1 milestone

Apr 20, 2022

Copy link

carsonbot commentedApr 20, 2022

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 6.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

hmoreau force-pushed theCASE_INSENSITIVE_FILE_LIST branch 2 times, most recently from4b9ad93 to5b30852Compare

April 20, 2022 19:28

GromNaN reviewed

Apr 20, 2022

View reviewed changes

Copy link

Member

GromNaN left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for this very complete PR with tests and docs.

I think we can improve the interface.

src/Symfony/Component/Finder/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/Finder/Finder.php Outdated

		* @see SortableIterator
		*/
		publicfunctionsortByName(bool$useNaturalSort =false):static
		publicfunctionsortByName(bool$useNaturalSort =false,bool$useCaseInsensitive =false):static

Copy link

Member

GromNaNApr 20, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 frombool tobool|int.

Instead, we can add the methodsortByNameCaseInsensitive(bool $useNaturalSort = false) (name to be discussed).

Copy link

ContributorAuthor

hmoreauApr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 aboutsortByCaseName(bool $useNaturalSort = false) which is closer to the php functionsstrcasecmpandstrnatcasecmp ?

Copy link

Member

GromNaNApr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

3rd alternative, theFinder::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 toMyEnum|\Closure $arg

Copy link

ContributorAuthor

hmoreauApr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Meanwhile I have moved the code tosortByCaseName(bool $useNaturalSort = false)

nicolas-grekas modified the milestones:6.1,6.2

Apr 21, 2022

stof reviewed

Apr 22, 2022

View reviewed changes

src/Symfony/Component/Finder/Iterator/SortableIterator.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Finder/Finder.php Outdated

		publicfunctionsortByCaseName(bool$useNaturalSort =false):static
		{
		$this->sort =$useNaturalSort ?Iterator\SortableIterator::SORT_BY_NAME_NATURAL_CASE_INSENSITIVE :
		Iterator\SortableIterator::SORT_BY_NAME_CASE_INSENSITIVE;

Copy link

Member

stofApr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this should stay on the same line

src/Symfony/Component/Finder/CHANGELOG.md Outdated

		CHANGELOG
		=========

		6.1

Copy link

Member

stofApr 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this will likely be 6.2 as we are past the feature freeze for 6.1

javiereguiluz reviewed

Apr 26, 2022

View reviewed changes

src/Symfony/Component/Finder/Finder.php OutdatedShow resolvedHide resolved

hmoreau force-pushed theCASE_INSENSITIVE_FILE_LIST branch from82d04f6 to2f26995Compare

April 27, 2022 08:48

Copy link

ContributorAuthor

hmoreau commentedApr 27, 2022

I made the change tosortByCaseInsensitiveName

fabpot requested changes

Jul 22, 2022

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you rebase on current 6.2 to eliminate the merge commits (we don't merge PRs with merge commits)?

src/Symfony/Component/Finder/Finder.phpShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jul 22, 2022

[Finder] Add case insensitive sort

7fdbb61

fabpot force-pushed theCASE_INSENSITIVE_FILE_LIST branch fromee5661b to7fdbb61Compare

July 22, 2022 12:41

fabpot approved these changes

Jul 22, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Jul 22, 2022

Copy link

Member

fabpot commentedJul 22, 2022

Thank you@hmoreau.

fabpot merged commit25c8806 intosymfony:6.2

Jul 22, 2022

Copy link

ContributorAuthor

hmoreau commentedJul 22, 2022

Thank you@fabpot ,@GromNaN ,@javiereguiluz ,@nicolas-grekas and@stof

hmoreau mentioned this pull request

Aug 9, 2022

[Finder] Add case insensitive sortsymfony/symfony-docs#17131

Merged

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Aug 9, 2022

minor#17131[Finder] Add case insensitive sort (hmoreau)

aeefc87

This PR was squashed before being merged into the 6.2 branch.Discussion----------[Finder] Add case insensitive sortAdd 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 tosymfony/symfony#46126 PRReplace#16735 PRCommits-------df9281f [Finder] Add case insensitive sort

fabpot mentioned this pull request

Oct 24, 2022

Release v6.2.0-BETA1#47972

Merged