Add default_order option to doctrine processors #13
Conversation
There was a problem hiding this comment.
Hey there @alekitto!
Thank you for this PR. Just left some comments 😄
| @@ -14,7 +14,7 @@ class StringToExpresionTransformer implements DataTransformerInterface | |||
|
|
|||
| public function __construct(?Grammar $grammar = null) | |||
| { | |||
| $this->grammar = $grammar ?? new Grammar(); | |||
| $this->grammar = $grammar ?? self::getGrammar(); | |||
There was a problem hiding this comment.
Instead of having in this transformer the static method getGrammar(), what do you think about making Grammar class singleton?
In this way we can have Grammar::getInstance()
| $value = '$order('.$value.')'; | ||
| } | ||
|
|
||
| $grammar = new Grammar(); |
There was a problem hiding this comment.
Instead of new Grammar() I would use the Grammar::getInstance(). Probably it could be interesting overriding that grammar. Using $this->grammar (and giving possibility to override that with a setter setGrammar(?Grammar $grammar)) could make this possible (like in the transformer above)
| /** | ||
| * @dataProvider provideParamsForDefaultOrder | ||
| */ | ||
| public function testShouldOrderByDefaultField(bool $exception, bool $valid, string $defaultOrder): void |
There was a problem hiding this comment.
I would prefer to have these tests separated instead a single.
Something like testOrderByDefaultFieldShouldThrowOnInvalidOptions and testOrderByDefaultFieldShouldWork
| /** | ||
| * @dataProvider provideParamsForDefaultOrder | ||
| */ | ||
| public function testShouldOrderByDefaultField(bool $exception, bool $valid, string $defaultOrder): void |
There was a problem hiding this comment.
Same as the ORM one: I would prefer to have these tests separated instead a single.
Something like testOrderByDefaultFieldShouldThrowOnInvalidOptions and testOrderByDefaultFieldShouldWork
alekitto
force-pushed
the
default_order
branch
from
76c9265 to
4f4b65b
Compare
Implement
default_orderoption.Fix #9