-
Notifications
You must be signed in to change notification settings - Fork 75
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
Many changes, more comments and types #109
Conversation
@@ -32,14 +32,14 @@ class EMongoDocument extends EMongoModel{ | |||
/** | |||
* Contains a list of fields that were projected, will only be taken into consideration | |||
* should _partial be true | |||
* @var array | |||
* @var array|string[] |
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.
I looked into whether string[] was a standard in PHP doc, can you give me a link which dictates it is?
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.
http://www.phpdoc.org/docs/latest/for-users/types.html
specified containing multiple types, the Type definition informs the reader of the type of each array element. Each element can be of any of the given types. Example: @return (int|string)[]
I advise you to use PhpStorm speeds up significantly. |
You must remember that even if I were to switch to phpStrom (of which Eclipse works really well for me and is free) many others will not be on it and PhpDoc does work differently outside of PhpStorm |
|
||
if(($pagination=$this->getPagination())!==false) | ||
// TODO Needs refactoring |
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.
In what way does it need refactoring?
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.
TODO code below does not work, I had commented out and set a mark for you, because I do not know what he's doing:
// if(isset($criteria['hint']) && (is_array($criteria['hint']) || is_string($criteria['hint'])))
// $this->_cursor->hint($criteria['hint']);
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.
Weird, that is definitely a bug, that hint command should be working.
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.
In the cursor is no method hint
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.
Hmm it should: http://www.php.net/manual/en/mongocursor.hint.php
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.
hmmm
I understand that perfectly, so comment and wrote @var array|string[] |
I did not come up with this format, and a link led to phpDoc where it says that it is not a violation of the standard. |
@@ -96,7 +99,7 @@ public function __call($name,$parameters){ | |||
* @example | |||
* | |||
* array( | |||
* '10_recently_published' => array( | |||
* 'published_10_recently' => array( |
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.
It is 10_recently_published reall that is being looked up, not sure if published_10_recently fits here
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.
All the keys in the array function scopes can be used as the function name! Function names should not start with a number!
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.
Ah yes I forgot about that limitation in PHP for a sec, hmm it still sounds clearer what it is if it is ten_recently_published
@@ -13,94 +13,100 @@ class EMongoSort extends CSort | |||
{ | |||
/** | |||
* @see yii/framework/web/CSort::resolveAttribute() |
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.
Should probably be CSort::resolveAttribute() to fit with the other way this is done
Ok been through it |
Many changes, more comments and types
Merged and fixed that last bit I talked about thanks :) Unit tests passing |
Good work on that by the way |
Good |
No description provided.