Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Interfaces #182

Closed
wants to merge 16 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

Haacked commented Jun 14, 2012

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.

Contributor

yorah commented Jun 14, 2012

@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 IRepository), but not necessarily on classes representing git objects (Branch, Commit ...). However, in order to make those classes test-friendly (i.e.: mockable), I just needed to add a protected empty constructor and make the needed properties virtual.

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.

Contributor

Haacked commented Jun 14, 2012

The idea is that an interface makes sense on classes exposing operations/behaviors (like IRepository),
but not necessarily on classes representing git objects (Branch, Commit ...).

Citation needed. :) Why doesn't an interface make sense on classes representing git objects? They also have behavior.

@nulltoken nulltoken and 1 other commented on an outdated diff Jun 15, 2012

LibGit2Sharp/DetachedHead.cs
{
- internal DetachedHead(Repository repo, Reference reference)
+ public DetachedHead(Repository repo, Reference reference)
@nulltoken

nulltoken Jun 15, 2012

Member

What makes this required?

@Haacked

Haacked Jun 15, 2012

Contributor

Not sure. I changed it back.

@nulltoken nulltoken commented on an outdated diff Jun 15, 2012

LibGit2Sharp/TreeEntryChanges.cs
@@ -5,7 +5,7 @@ namespace LibGit2Sharp
/// </summary>
public class TreeEntryChanges : Changes
{
- internal TreeEntryChanges(string path, Mode mode, ObjectId oid, ChangeKind status, string oldPath, Mode oldMode, ObjectId oldOid, bool isBinaryComparison)
+ public TreeEntryChanges(string path, Mode mode, ObjectId oid, ChangeKind status, string oldPath, Mode oldMode, ObjectId oldOid, bool isBinaryComparison)
@nulltoken

nulltoken Jun 15, 2012

Member

What makes this required?

Contributor

yorah commented Jun 15, 2012

Citation needed. :)

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:

  • When I create an interface, and there is only one implementation of this interface, I tend to think of it as a code smell. I usually take a step back and check if I really need it.
  • For mocking purposes, the protected constructor/virtual members seems to completely meet the need, while not extending the exposed surface of the API.

Why doesn't an interface make sense on classes representing git objects? They also have behavior.

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?

@nulltoken nulltoken commented on the diff Jun 15, 2012

LibGit2Sharp.Tests/BranchFixture.cs
Assert.NotNull(branch2);
Assert.Equal("br2", branch2.Name);
Assert.Equal(branch, branch2);
- Assert.True((branch2 == branch));
+ Assert.True(((Branch)branch2 == (Branch)branch));
@nulltoken

nulltoken Jun 15, 2012

Member

I'm not very keen on this change.

@Haacked

Haacked Jun 15, 2012

Contributor

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?

@nulltoken nulltoken commented on the diff Jun 15, 2012

LibGit2Sharp.Tests/ReferenceFixture.cs
Assert.Equal("refs/heads/master", head2.CanonicalName);
Assert.NotNull(head2.Tip);
- Assert.Equal(head.ResolveToDirectReference().Target, head2.Tip);
+ Assert.Equal(head.ResolveToDirectReference().Target, (GitObject)head2.Tip);
@nulltoken

nulltoken Jun 15, 2012

Member

Why the cast?

@tclem

tclem Jun 15, 2012

Member

Tip is a ICommit

@nulltoken nulltoken commented on the diff Jun 15, 2012

LibGit2Sharp/BlobExtensions.cs
@@ -0,0 +1,17 @@
+using System.Text;
+
+namespace LibGit2Sharp
+{
+ public static class BlobExtensions
@nulltoken

nulltoken Jun 15, 2012

Member

You're right! Those methods should be moved out of the Blob type.

@tclem tclem commented on the diff Jun 15, 2012

LibGit2Sharp.Tests/BranchFixture.cs
@@ -238,7 +238,7 @@ public void CanLookupLocalBranch()
{
using (var repo = new Repository(BareTestRepoPath))
{
- Branch master = repo.Branches["master"];
+ IBranch master = repo.Branches["master"];
@tclem

tclem Jun 15, 2012

Member

If you make these just var it will work for both cases.

Contributor

Haacked commented Jun 15, 2012

@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!

@Haacked Haacked closed this Jun 15, 2012

Contributor

yorah commented Jun 15, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment