Where group #186
Where group #186
Conversation
Created the initial function but came across an issue where the $whereClause doesn't work in the $where method when using an OR combiner, so need to fix that first
Methods and functions complete but failed testing in phpunit so needing to fix
PHPUnit tests succeeded.
Cleaned up trailing combiner in tests
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
lib/ezFunctions.php Outdated
| * Creates an equality comparison expression with the given arguments. | ||
| */ | ||
| function eq($x, $y, $and = null, ...$args) | ||
| function eq($x, $y, $and = null, $group = null, ...$args) |
There was a problem hiding this comment.
Method eq has 5 arguments (exceeds 4 allowed). Consider refactoring.
There was a problem hiding this comment.
All these bring in another required field? Whereas ...$args already covers that. You should try adding additional logic in the other methods instead to get the same results you are after.
The logic could be some kind of token/trigger word, to extract to get the desired action.
This is far beyond a breaking change, unnecessary, the logic should include a way for the current code to continue to work. The current tests should still pass.
lib/ezFunctions.php Outdated
| * Creates a non equality comparison expression with the given arguments. | ||
| */ | ||
| function neq($x, $y, $and = null, ...$args) | ||
| function neq($x, $y, $and = null, $group = null, ...$args) |
There was a problem hiding this comment.
Method neq has 5 arguments (exceeds 4 allowed). Consider refactoring.
lib/ezFunctions.php Outdated
| * Creates the other non equality comparison expression with the given arguments. | ||
| */ | ||
| function ne($x, $y, $and = null, ...$args) | ||
| function ne($x, $y, $and = null, $group = null, ...$args) |
There was a problem hiding this comment.
Method ne has 5 arguments (exceeds 4 allowed). Consider refactoring.
lib/ezFunctions.php Outdated
| * Creates a lower-than comparison expression with the given arguments. | ||
| */ | ||
| function lt($x, $y, $and = null, ...$args) | ||
| function lt($x, $y, $and = null, $group = null, ...$args) |
There was a problem hiding this comment.
Method lt has 5 arguments (exceeds 4 allowed). Consider refactoring.
lib/ezFunctions.php Outdated
| * Creates a lower-than-equal comparison expression with the given arguments. | ||
| */ | ||
| function lte($x, $y, $and = null, ...$args) | ||
| function lte($x, $y, $and = null, $group = null, ...$args) |
There was a problem hiding this comment.
Method lte has 5 arguments (exceeds 4 allowed). Consider refactoring.
| you will need to come up with a solution for the bc, to have tests pass as is. |
Changed from breaking changes to use the existing extra `$args`. All existing PHPUnit tests run without failing.
lib/ezFunctions.php Outdated
| : false; | ||
| } | ||
| | ||
| function flattenWhereConditions($whereConditions) |
There was a problem hiding this comment.
Function flattenWhereConditions has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.
There was a problem hiding this comment.
This function should be in the class method being used in. It's a private helper function that should not be called directly by user/developer.
There was a problem hiding this comment.
Very good point. I have moved this to the ezQuery file as a private method.
lib/ezQuery.php Outdated
| } | ||
| } | ||
| | ||
| public function whereGroup(...$whereConditions) |
There was a problem hiding this comment.
Function whereGroup has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
There was a problem hiding this comment.
What you think about just calling it group or grouping?
The only place it can be used in is with where already.
Repeating the same word within the same function call trying to stay away from, and keep naming somewhat similar to regular SQL dialect.
There was a problem hiding this comment.
Good idea. Have updated the code and tests. I didn't want to call it group as that felt too close to groupBy for me and I thought it might get confused, but I like grouping.
Incorrectly made a modification to the return of the `where` method instead of the `whereGroup` method
Thanks, I didn't even really think of using the existing Updated the code to use the existing options to avoid breaking changes. |
| You will need to re-sync with master branch, the CI travis and appveyor systems and github see a conflicting file, and Travis still has a fail. |
Synced with master branch to reduce conflicts
Synced with master branch to reduce conflicts
Thanks. Still learning how this all works with open source and larger projects. I've only really used GitHub for small personal projects until recently. Both CI checks passed :) |
Changed the method from `whereGroup` to `grouping` and moved the flatten function to be a private method in the `ezQuery` file as per @techno-express recommendations.
| } | ||
| } | ||
| | ||
| public function grouping(...$whereConditions) |
There was a problem hiding this comment.
Function grouping has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
Added a brief example in the readme file underneath the shortcut methods. Will add a detailed example in the wiki once approved.
2 participants
Resolves #164
UPDATE: There are no longer breaking changes
Example code from
pdo\pdo_mysqlTest.phpfile:or another example without the function
The combiner will add itself at the end of the group.