Working on code quality #148
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JarJak
requested changes
Nov 20, 2018
|
PR updated. |
JarJak
approved these changes
Nov 21, 2018
|
ECS is in agreement with the code now.. On to PHPstan! |
JarJak
requested changes
Nov 21, 2018
|
@JarJak My latest commit bumps Doctrine to 2.10. It doesn't have the deprecation notice anymore, and it also resolves this issue with Travis:
|
|
I really like how changes are going and I love how Laravel Collections are used here (much better than using array_* functions) :) |
|
Wow, all green! 🥇 Now we can put phpstan level up :D (but in another PR) |
JarJak
requested changes
Nov 21, 2018
JarJak
requested changes
Nov 22, 2018
|
I really don't like all those "magic" methods in ContentMagicTrait. Can we at least rename them to be less magical? :) |
JarJak
requested changes
Nov 22, 2018
|
Agreed. I’ll make a ticket issue for that..
Also agreed on your other point: This PR made sure Travis is green, but
it’s getting quite large. Let’s get this in, and do additional improvements
in separate issues.
Feel free to merge. ;-)
On Thu, 22 Nov 2018 at 11:36, Jarek Jakubowski ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/Entity/ContentMagicTraits.php
<#148 (comment)>:
> @@ -55,6 +55,17 @@ public function get(string $name, array $arguments = [])
}
}
+ public function has(string $name): bool
+ {
+ foreach ($this->fields as $field) {
+ if ($field->getName() === $name) {
if we could make $this->fields a name-indexed array, using this method be
much faster (simple isset instead of foreaching).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#148 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABv5kczhAMf44V1c8p4uaTleLjQ57NZAks5uxn4xgaJpZM4Yp3xL>
.
--
Bob den Otter - bobdenotter@gmail.com <bob@twokings.nl> - @bopp
|
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From "Found 37 errors that needs to be fixed manually" down to "Found 27 errors that needs to be fixed manually"