search

Code2Gether-Discord / Code2Gether-Discord-Bot

A Discord bot for the Code2Gether server
3 stars 1 forks source link

Fixed issue not creating more than 1 project #29

Closed nickmartin1ee7 closed 3 years ago

nickmartin1ee7 commented 3 years ago

Closes #28

srjheam commented 3 years ago

@nickmartin1ee7 Looks good, is there a way to test this though?

nickmartin1ee7 commented 3 years ago

@nickmartin1ee7 Looks good, is there a way to test this though?

You're right. There should be a unit test to cover this scenario. Good catch!

srjheam commented 3 years ago

I'm thinking if I can do it, but what's the meaning of FakeProjectManager class on Tests? Because just using new ProjectManager(...) would do the work and it prevents the devs from changing the same code on multiple areas. If the fake is absolutely necessary, IMHO its entire code should be Inherit from Projects Manager because it knows what to do.

Do you agree with me, @nickmartin1ee7?

nickmartin1ee7 commented 3 years ago

Yeah, that's actually a testable class on its own. Making a fake for that was a mistake since we already pass in a fake repository. Not to mention I broke DRY principle making it.

srjheam commented 3 years ago

Yeah, that's actually a testable class on its own. Making a fake for that was a mistake since we already pass in a fake repository. Not to mention I broke DRY principle making it.

May I open another two issues, one to remove fake classes and one to test the CreateProject method?

nickmartin1ee7 commented 3 years ago

Yeah, that's actually a testable class on its own. Making a fake for that was a mistake since we already pass in a fake repository. Not to mention I broke DRY principle making it.

May I open another two issues, one to remove fake classes and one to test the CreateProject method?

Yeah. Since the create project is dependant on the other, I'd make it a single refactor user story.

nickmartin1ee7 commented 3 years ago

@srjheam Can you review this so we can close this PR, and delete this branch?