[5.5] removed unnecessary assignment of variable #22115
Conversation
| $data = $data->toArray(); | ||
| } elseif ($data instanceof JsonSerializable) { | ||
| $data = $data->jsonSerialize(); | ||
| if (! is_array($data)) { |
There was a problem hiding this comment.
Can't we just remove this check completely?
There was a problem hiding this comment.
Wouldn't that be a breaking change? I kept it because it matched the behaviour before removing it.
There was a problem hiding this comment.
How would it break anything? The other two checks don't match arrays
There was a problem hiding this comment.
I know..but didn't want to risk it incase there is any edge case.
There was a problem hiding this comment.
There's no cases where an instance of Collection, Arrayable or JsonSerializable will pass is_array
There was a problem hiding this comment.
i know.. but these things often have some weird edge cases 😆
| } elseif ($data instanceof JsonSerializable) { | ||
| $data = $data->jsonSerialize(); | ||
| } | ||
| if ($data instanceof Arrayable || $data instanceof Collection) { |
There was a problem hiding this comment.
if ($data instanceof Arrayable) {The Collection class already implements the Arrayable interface.
|
Someone tried refactoring this method a few weeks ago and Taylor responded: #21526 (comment) |
|
You'd think they cared more for quality code. |
|
@jordyvandomselaar when you're alone handling dozens of weekly PRs changing the code used by millions of people around the globe everyday, striving not to introduce new bugs or useless breaking changes, it's completely understandable why small changes like these gets ignored. |
|
Absolutely. They've been burned many times by seemingly innocent small changes meant to improve readability. |
|
Ironically we have had breaking changes and other bugs from more feature code than readability fixes. The code was structured keeping that in mind that it doesn't change any behaviour any introduce further bugs. By this logic we shouldn't be doing code refactorings at all and we know how that goes. Also I know there are a lot of PRs coming in everyday, but there is no hurry to merge them quickly. |
|
Having many PR's shouldn't be an excuse to keep low quality code lingering around. PR's like these could be put on low priority for someone else to do in their spare time. Refactoring old code doesn't only make things more readable, it also makes it less likely to break when adding new features or fixing other bugs. It'd be a shame if the entire core fell flat because some old code isn't behaving the way it should. You could create a test branch where things get merged provided they have proper unit tests, so the community can test the framework. It'd be like a beta. Then, anyone on the team can test it thoroughly before it makes it to master. |
|
@jordyvandomselaar If you find a low-quality code, please do open a PR for people to look over. This PR isn't the case. This isn't a low-quality code. It's clear and readable. There has been some great refactoring PRs that got merged for those reasons you pointed out. As per your thoughts on how it should be done, that's not how the Laravel ecosystem is maintained. |
|
Duh. We know that's not how the Laravel ecosystem is maintained that's why we are pointing it out 😛 |
|
Useless code equals low quality code. And, just because that's not how it's done right now doesn't mean it shouldn't ever happen. It would benefit the framework, and you with it. |
|
@jordyvandomselaar this is precisely one of the great reasons I see Taylor closing these kinds of PR. Opinionated. You think the code is useless, I don't. For instance, this version of the refactoring doesn't have the useless code, yet it's less readable. |
|
That code may be readable, but the change isn't comprising on its readability. Also I showed the original code.
Even Phpstorm complains that it is useless. |
|
The code being useless isn't entirely opinion based. It really doesn't do anything at all. It doesn't make it more readable, it doesn't make things more performant, it doesn't fix anything; it does absolutely nothing at all. Not trying to get personal here, but it seems you're more opinionated on this than anyone. Reason it feels like that is because you're defending code that should never have existed in the first place. |
The useless code is more readable than this. The final version of this PR I would agree that ended up being better than the current implementation, but the point I'm trying to make is about 2 lines of refactoring in general. The risks outweight the benefits. |
|
That I can agree with. I'll see what I can cook up sometime, or leave it alltogether. This case in particular doesn't bother me too much; the explanation given for it not to be merged annoys me. |
8 participants
removed unnecessary assignment of