Skip to content
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

Question on ClassInfoTrait #475

Closed
theofidry opened this issue Mar 19, 2016 · 7 comments
Closed

Question on ClassInfoTrait #475

theofidry opened this issue Mar 19, 2016 · 7 comments

Comments

@theofidry
Copy link
Contributor

ClassInfoTrait

Is there any point to have it as a trait? It only has the following method:

private function getObjectClass($object)
    {
        return class_exists('Doctrine\Common\Util\ClassUtils') ? ClassUtils::getClass($object) : get_class($object);
    }

IMO this could be a public static function instead: having it as a trait only duplicate the method on which classes using it and it does only PHP function calls or static calls, so be it a private method in a trait or a public static method, it doesn't change anything in terms of testability (for the class using it), while make this util testable.

@dunglas
Copy link
Member

dunglas commented Mar 19, 2016

Traits are easily testable? I don't see any benefit to remove the trait for adding a static method. 👎 for me.

@theofidry
Copy link
Contributor Author

Traits are easily testable?

With a private method it requires to have create a test class using this trait and having a public method calling it directly. Doable but slightly more annoying.

In any case I will add a test for it, but I really don't see the advantage of using a trait in this case (well, it's not like I'm fond of static calls either meh).

@dunglas
Copy link
Member

dunglas commented Mar 19, 2016

You can use the use { method as public; } syntax. It's not a big deal. IMO traits are better than public static methods. Btw, if it's not already the case, I would make this trait @internal.

@theofidry
Copy link
Contributor Author

You can use the use { method as public; } syntax

Ah nice one.

IMO traits are better than public static methods

Didn't make my opinion on it yet :p

I would make this trait @internal.

It is don't worry

@theofidry
Copy link
Contributor Author

Anyway will just take care of adding a test for it then

@teohhanhui
Copy link
Contributor

I'm of the opinion that we don't need a test for this. It'll only be testing Doctrine's method, or get_class. Completely superfluous.

@theofidry
Copy link
Contributor Author

Fair enough.

Anyway we need to get phpspec/prophecy#263 or phpspec/prophecy#264 merged for proper unit testing 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants