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

[Finder] added "use natural sort" option#27024

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:masterfromvyshkant:finder_natural_ordering

May 30, 2018

Merged

[Finder] added "use natural sort" option#27024

fabpot merged 1 commit intosymfony:masterfromvyshkant:finder_natural_ordering

May 30, 2018

Conversation

Copy link

Contributor

vyshkant commentedApr 23, 2018•
edited
Loading

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26930
License	MIT
Doc PR	symfony/symfony-docs#9671

Added$useNaturalSort optional argument toFinder::sortByName() method. If it is specified and equals totrue,"natural sort order" algorithm will be applied, which means thatstrnatcmp function will be used instead ofstrcmp (see#26930 for details).

carsonbot added Status: Needs Review Finder Feature labels

Apr 23, 2018

nicolas-grekas added this to thenext milestone

Apr 24, 2018

nicolas-grekas requested changes

Apr 24, 2018

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Almost good, thank you.

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

		* @see SortableIterator
		*/
		publicfunctionsortByName()
		publicfunctionsortByName($naturalOrdering =false)

Copy link

Member

nicolas-grekasApr 24, 2018

Choose a reason for hiding this comment

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

for BC concerns, this should be handled usingfunc_num_args()/func_get_arg(0) (adding a new argument breaks child classes).
The signature should then be written assortByName(/* bool $useNaturalOrdering = false */)

Copy link

Member

javiereguiluzApr 24, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Most of the times"sort" and "order" are interchangeable. However, given that our method is calledsortByName, we use a$this->sort property and PHP calls this "natsort" for "natural sort" ... could we please rename$useNaturalOrdering to$useNaturalSort ?

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


		* deprecated`Symfony\Component\Finder\Iterator\FilterIterator`
		* added Finder::hasResults() method to check if any results were found
		* added $naturalOrdering option to Finder::sortByName() method

Copy link

Member

nicolas-grekasApr 24, 2018

Choose a reason for hiding this comment

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

$useNaturalOrdering looks more explicit to me

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

		*
		* This can be slow as all the matching files and directories must be retrieved for comparison.
		*
		* @param bool $naturalOrdering Whether to use natural ordering or not

Copy link

Member

nicolas-grekasApr 24, 2018

Choose a reason for hiding this comment

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

@param bool $useNaturalOrdering Whether to use natural ordering or not, disabled by default

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

		publicfunctionsortByName($naturalOrdering =false)
		{
		$this->sort =Iterator\SortableIterator::SORT_BY_NAME;
		$this->sort =$naturalOrdering

Copy link

Member

nicolas-grekasApr 24, 2018

Choose a reason for hiding this comment

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

our CS favors single lines, please adjust

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

Apr 24, 2018

Copy link

Member

nicolas-grekas commentedApr 24, 2018•
edited
Loading

New features should target master (aka 4.2)!

Copy link

ContributorAuthor

vyshkant commentedApr 24, 2018

New features should target master (aka 4.2)!

@nicolas-grekas So I have to add a message toCHANGELOG.md under 4.2.0 section (not under 4.1.0)?

Copy link

Member

nicolas-grekas commentedApr 24, 2018

add a message to CHANGELOG.md under 4.2.0 section

yes please

vyshkant changed the base branch from3.4 tomaster

April 24, 2018 11:55

vyshkant changed the title~~[Finder] added "natural ordering" sort option~~[Finder] added "natural sort" option

Apr 24, 2018

vyshkant changed the title~~[Finder] added "natural sort" option~~[Finder] added "use natural sort" option

Apr 24, 2018

nicolas-grekas reviewed

Apr 24, 2018

View reviewed changes

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

		publicfunctionsortByName(/* bool $useNaturalSort = false */)
		{
		$this->sort =Iterator\SortableIterator::SORT_BY_NAME;
		$useNaturalSort =1 ===func_num_args() &&true ===func_get_arg(0);

Copy link

Member

nicolas-grekasApr 24, 2018•
edited
Loading

Choose a reason for hiding this comment

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

I'd be a bit more lax here:0 < func_num_args() && func_get_arg(0)

[Finder] added "use natural sort" option

e697c7d

Copy link

ContributorAuthor

vyshkant commentedApr 24, 2018

Does anybody have any idea why AppVeyor build has been failed?

nicolas-grekas approved these changes

Apr 24, 2018

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(failure unrelated)

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

Apr 24, 2018

fabpot approved these changes

May 30, 2018

View reviewed changes

Copy link

Member

fabpot commentedMay 30, 2018

Thank you@vyshkant.

fabpot merged commite697c7d intosymfony:master

May 30, 2018

fabpot added a commit that referenced this pull request

May 30, 2018

feature#27024[Finder] added "use natural sort" option (vyshkant)

3bade96

This PR was merged into the 4.2-dev branch.Discussion----------[Finder] added "use natural sort" option| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#26930| License       | MIT| Doc PR        |symfony/symfony-docs#9671Added `$useNaturalSort` optional argument to `Finder::sortByName()` method. If it is specified and equals to `true`, ["natural sort order" algorithm](https://en.wikipedia.org/wiki/Natural_sort_order) will be applied, which means that `strnatcmp` function will be used instead of `strcmp` (see#26930 for details).Commits-------e697c7d [Finder] added "use natural sort" option

nicolas-grekas modified the milestones:next,4.2

Nov 1, 2018

This was referencedNov 3, 2018

Release v4.2.0-BETA1#29072

Closed

Release v4.2.0-BETA1#29073

Merged