Cake5 #112
Cake5 #112
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## cake5 #112 +/- ##
========================================
Coverage ? 81.51%
Complexity ? 394
========================================
Files ? 13
Lines ? 1098
Branches ? 0
========================================
Hits ? 895
Misses ? 203
Partials ? 0 ☔ View full report in Codecov by Sentry. |
|
As I mentioned on the related issue your PR needs to target the |
|
You just changed the branch target without rebasing and fixing merge conflicts 🙂 |
|
Sorry, I made a typo in my previous comment, you need to rebase upon |
…part of the psalm errors fixed
…part of the psalm errors fixed
Rebased |
| @@ -659,6 +661,9 @@ public function findAll(Query $query, array $options): Query | |||
| */ | |||
| public function findList(Query $query, array $options): Query | |||
There was a problem hiding this comment.
This method needs to be updated similar to core's Table::findList()
There was a problem hiding this comment.
Right, will change that. I asumed (but that turns out to be wrong) that the first PR fixed most of these issues.
There was a problem hiding this comment.
Seeing that the findList method in core returns a SelectQuery. Do you want me to implement the different query types core has here as well? Or just the way this method works? @ADmad
There was a problem hiding this comment.
Yes eventually do I want to use separate query classes for this plugin too. That would allow us to clean up the types for the Query::$_results property. Currently it can be of multiple different types which makes the code messy as we have to add type checks wherever it's used, as evident from your changes.
… psalm constraint. Added cache files and diff file for phpcs to gitignore.
| @@ -659,6 +661,9 @@ public function findAll(Query $query, array $options): Query | |||
| */ | |||
| public function findList(Query $query, array $options): Query | |||
There was a problem hiding this comment.
Yes eventually do I want to use separate query classes for this plugin too. That would allow us to clean up the types for the Query::$_results property. Currently it can be of multiple different types which makes the code messy as we have to add type checks wherever it's used, as evident from your changes.
|
I don't really have the time or expertise to implement the seperate query classes from core. Could this be picked up in a seperate issue? |
Yup |
An updated version of MolbioUnige's PR, to fix the pipeline and fix various typing issues.
PR into MolbioUnigue's repo: MolbioUnige#1