selective-php / test-traits

Test Traits
MIT License
13 stars 2 forks source link

Include the traits a trait depends on #13

Closed samuelgfeller closed 10 months ago

samuelgfeller commented 10 months ago

The HttpJsonTestTrait requires HttpTestTrait for the createJsonRequest() function to work.

It's also written in the php class doc that it requires HttpTestTrait and ArrayTestTrait.

Why not include them into the trait like this:

trait HttpJsonTestTrait
{
    use HttpTestTrait;
    use ArrayTestTrait;

    // ...
}

I do understand that it makes them more “lightweight” in case the HttpJsonTestTrait is used without the need for createJsonRequest(). But these traits are so small, and it's such an obvious dependency, that I find it more intuitive if people hadn't to manually also include the traits this trait depends on in their test case.

Can I make a PR?

odan commented 10 months ago

This is for the sake of better "composability". This allows other developers to implement their own createJsonRequest if they need to.

samuelgfeller commented 10 months ago

Do you mean they can make their own createRequest that the function createJsonRequest would use over the one from HttpTestTrait?

That does make sense, if their logic of creating a request is different, and they still want to use the HttpJsonTestTrait and createJsonRequest.

Although I understand this design decision, I think that the extra complexity that is required from everyone (to include HttpTestTrait when wanting to use HttpJsonTestTrait) is not worth the added convenience for the people that want to do it differently. They could very well make their custom createJsonRequest function in another http json test trait that uses their request creation logic.

odan commented 10 months ago

The idea is that you can use and combine these traits as you want. For example if you always need the same two traits, such as HttpTestTraitand and HttpJsonTestTrait , then use them both in your application specific test trait and use that trait within the project. I hope this explains it better.

samuelgfeller commented 10 months ago

I guess I would have designed the library differently so that the users don't have to create an application specific test trait to use HttpJsonTestTrait without also having to always include HttpTestTrait. But you answered the question and don't seem keen on changing this, so I'm closing the issue.

Thanks for your insight.