-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Orderby ability #9
Conversation
@@ -369,6 +365,19 @@ protected function executeScript($db, $type, $columnName = null) | |||
*/ | |||
private function findByPk($db, $type, $columnName = null) | |||
{ | |||
if (!empty($this->orderBy) and in_array($type, ['All', 'One', 'Count', 'Column'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and
→ &&
really cool, will check it in detail later. Could you add some unit tests? At least these two functions should be removed, which will add tests for some sorting: |
@cebe, I will try to add unit tests for the week |
Find some bug... all values will be sort like strings. For example we have numeric pk id (1,2,3,4...11,12...21,22). The result for by id will be Don't have any idea to solve this. May be test is_numeric for values and use type casting for numeric values |
Hi all again. Tried to fix all bugs and errors. Remove duplicated test consisted orderBy call. Tell me pls, should I add test for this case: |
@@ -369,6 +366,21 @@ protected function executeScript($db, $type, $columnName = null) | |||
*/ | |||
private function findByPk($db, $type, $columnName = null) | |||
{ | |||
if (!empty($this->orderBy) && in_array($type, ['All', 'One', 'Count', 'Column'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not need sorting for type 'Count'.
And this expression must be assigned to a variable because it's duplicated 2 times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, fixed
@@ -369,6 +366,22 @@ protected function executeScript($db, $type, $columnName = null) | |||
*/ | |||
private function findByPk($db, $type, $columnName = null) | |||
{ | |||
$typesForOrder = ['All', 'One', 'Column']; | |||
if (!empty($this->orderBy) && in_array($type, $typesForOrder)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$needSort = !empty($this->orderBy) && in_array($type, ['All', 'One', 'Column']);
if ($needSort) {
...
}
Hi all, |
Any changes? |
@@ -369,6 +366,22 @@ protected function executeScript($db, $type, $columnName = null) | |||
*/ | |||
private function findByPk($db, $type, $columnName = null) | |||
{ | |||
$needSort = !empty($this->orderBy) && in_array($type, ['All', 'One', 'Column']); | |||
if ($needSort) { | |||
if (count($this->orderBy) > 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need {
and }
even for one liners.
Hey, what about merging it? |
Excuse me, have repo team plan to merge this pr or something wrong with it? |
Finally found the time to review this, merged, Thank you @valinurovam! |
Hi all.
Im tried to add orderby ability for yii2-redis. It works for me. Review please.