API review

[happyr]

API review

[Nyholm]

I have noticed a few things. Please discuss them, I love to hear some inputs.

A) The Expression interface should be renamed to "Filter" to make sense. Also the method names referring to that interface should (of course) be renamed.
B) The both Modifier interfaces should be renamed to something more descriptive. Like QueryModifier and ResultModifier. I'm getting confused 😕
C) I want to rewrite BaseSpecification somehow.. But I think I need to try it out more first.. I'll get back..
D) Nice @cakper for running return $query->execute(); instead of return $query->getResult($query->getHydrationMode()).

[Nyholm]

This is another spec:

class AdvertsWeShouldClose extends BaseSpecification
{
    public function __construct($dqlAlias = null)
    {
        parent::__construct($dqlAlias);
        $this->spec = Spec::andX(
            Spec::eq('ended', 0),
            Spec::orX(
                Spec::lt('endDate', new \DateTime()),
                Spec::andX(
                    Spec::isNull('endDate'),
                    Spec::lt('startDate', new \DateTime('-4weeks'))
                )
            )
        );
    }

    /**
     * @return Expression
     */
    public function getWrappedExpression()
    {
        return $this->spec;
    }

    /**
     * @return Modifier
     */
    public function getWrappedModifier()
    {
        return $this->spec;
    }
}

This one should be valid. But LogicX does require a Specification. A `Expression´ should also work here.. I'm making a PR.

[cakper]

A) The Expression interface should be renamed to "Filter" to make sense. Also the method names referring to that interface should (of course) be renamed.

agree :)

B) The both Modifier interfaces should be renamed to something more descriptive. Like QueryModifier and ResultModifier. I'm getting confused

If you (author) are confused than definitely we should rename it because it will make even more confusion to users.

[Nyholm]

Okey. Thank you @cakper for your input. I'll write a PR for A & B

[cakper]

I'm just doing PR for the first one ;)

[Nyholm]

I'll do the second one then =)

[Nyholm]

It feels like every time we make an effort on this repo we find more stuff to do... But with the PRs above and a PR for (A) I feel pretty happy with the result. 😄

[Nyholm]

All the raised issued has gotten a PR and they are all merged. Thank you!