Interfaces #182
Interfaces #182
Conversation
Propagated the interface through the API.
These unit tests isolate testing the extension methods and don't require creating actual repositories on disk. They are meant to demonstrate how using interfaces can help isolate code being tested.
|
@haacked This topic triggered my curiosity, and after playing a bit with Moq I think I have a slightly different proposal, which I think would fit the need to make LibGit2Sharp more testable. The idea is that an interface makes sense on classes exposing operations/behaviors (like Would you please be so kind as to take a look at my interfaces branch, and tell me what you think about it? Please note that I only implemented this proposal on a few classes, in order to get your feedback. In case you like it, I will gladly extend the scope to cover the same classes that you did. |
Citation needed. :) Why doesn't an interface make sense on classes representing git objects? They also have behavior. |
| { | ||
| internal DetachedHead(Repository repo, Reference reference) | ||
| public DetachedHead(Repository repo, Reference reference) |
There was a problem hiding this comment.
Not sure. I changed it back.
I had no citation in mind when I wrote this, I was merely voicing out my opinion :) I tried to organize my thoughts, and here is why I proposed the "protected constructor & virtual members" approach:
Actually, for me, they are rather just objects used to hold data related to the content of a repository. The real operation/behavior/action logic is accessed through the Repository. On a separate note, I just realized I commented on your PR, instead of commenting on #138, where you were already having discussions about the same topic. Would you like to switch back there? |
| Assert.NotNull(branch2); | ||
| Assert.Equal("br2", branch2.Name); | ||
|
|
||
| Assert.Equal(branch, branch2); | ||
| Assert.True((branch2 == branch)); | ||
| Assert.True(((Branch)branch2 == (Branch)branch)); |
There was a problem hiding this comment.
I'm not very keen on this change.
There was a problem hiding this comment.
Unfortunately interfaces don't allow operator overloads as extension methods. I could just delete this line or change it to:
Assert.True(branch2.Equals(branch));
Which would you prefer?
Per code review comments
|
@yorah OK, I had a chat with @nulltoken about this and long story short, let's go with the approach you described. Interfaces vs classes with virtuals is a debate as old as time and I'm just not too keen to rehash it over and over. The most important thing to me is to get changes in LibGit2Sharp that will enable our testing scenarios and I believe your changes meet that need and fit the design sensibilities of LibGit2Sharp. Bonus, they are certainly much less breaking than my changes are. :) Are you going to tackle this? Do you need any help from me? Thanks! |
|
It would be a pleasure for me to tackle this! I will make the changes based on what you did (the assumption being that everything you put as interfaces in this PR should be testable), so the scope should be identical. I will make a pull request tomorrow. It would be nice of you to review it when it's done. |
This pull request adds some interfaces to more of the commonly used types. In one case, instead of an interface, I just changed the ctor from internal to public.
I also added unit tests for RepositoryExtensions that don't require creating an actual repository as a demonstration of how these interfaces allow isolation in unit tests.