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

Disallow ksorting lists #7019

Merged
merged 6 commits into from Jan 2, 2022
Merged

Disallow ksorting lists #7019

merged 6 commits into from Jan 2, 2022

Conversation

danog
Copy link
Collaborator

@danog danog commented Nov 29, 2021

Fixes #7018.
Settled on the same logic used for array_values, as lists can only be constructed by appending elements to an array or by using array_values itself, in both cases they are sorted by default.

@orklah
Copy link
Collaborator

orklah commented Nov 29, 2021

I'm not a fan of this one, ksort is hardly a function to transform types, so RedundantCast feels very weird here... However, I couldn't find a more adapted issue and I'm not sure this case is common enough to create a new issue...

@weirdan
Copy link
Collaborator

weirdan commented Nov 29, 2021

I'd suggest to add RedundantFunctionCall for this kind of issues. Also, this looks very useful to me (as are other dead/useless code checks).

@orklah
Copy link
Collaborator

orklah commented Nov 29, 2021

I was not clear, I meant that I was not a fan of this being classified as RedundantCast but the feature seem fine though.

I'm okay with a RedundantFunctionCall, thinking about it more, this could indeed be reused (a dumb example, but a call to max() with a single value could fit in there).

@orklah orklah marked this pull request as draft December 14, 2021 23:54
@danog danog marked this pull request as ready for review December 30, 2021 10:37
@danog
Copy link
Collaborator Author

danog commented Dec 30, 2021

Done!

@orklah
Copy link
Collaborator

orklah commented Dec 30, 2021

Cool! Thanks!

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Dec 30, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 30, 2021

@weirdan Can I let you merge that and updating the baseline for phpunit?

@weirdan weirdan added release:feature The PR will be included in 'Features' section of the release notes and removed release:feature The PR will be included in 'Features' section of the release notes labels Jan 2, 2022
@weirdan weirdan merged commit 36d5a2a into vimeo:master Jan 2, 2022
@weirdan
Copy link
Collaborator

weirdan commented Jan 2, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: It makes no sense to ksort a list-like keyed array
3 participants