Skip to content

Refactor away TemporaryCloneOfTestRepo#386

Merged
nulltoken merged 1 commit into
libgit2:vNextfrom
dahlbyk:CloneForTest
Apr 8, 2013
Merged

Refactor away TemporaryCloneOfTestRepo#386
nulltoken merged 1 commit into
libgit2:vNextfrom
dahlbyk:CloneForTest

Conversation

@dahlbyk

@dahlbyk dahlbyk commented Apr 7, 2013

Copy link
Copy Markdown
Member

Following up on this conversation: #312 (diff)

Before merging I'd suggest this be squashed into a single commit. For consistency usage, I'd suggest we either go with the approach suggested by the third commit (a Clone___TestRepo() corresponding to each test repo), or replace CloneBareTestRepo() with Clone(BareTestRepoPath). I think I prefer the former (thus including it here).

@nulltoken

Copy link
Copy Markdown
Member

I think I prefer the former (thus including it here).

So do I.

Before merging I'd suggest this be squashed into a single commit.

Could you take care of this as well?

@nulltoken

Copy link
Copy Markdown
Member

Just one nitpick:

Could you please make the type explicit?

string path = CloneStandardTestRepo();

instead of

var path = CloneStandardTestRepo();

@dahlbyk

dahlbyk commented Apr 8, 2013

Copy link
Copy Markdown
Member Author

Could you take care of this as well?

Done

Could you please make the type explicit?

Done

@nulltoken
nulltoken merged commit 8738461 into libgit2:vNext Apr 8, 2013
@nulltoken

Copy link
Copy Markdown
Member

Before merging

Done

@nulltoken

Copy link
Copy Markdown
Member

😍

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

Successfully merging this pull request may close these issues.

2 participants