-
Notifications
You must be signed in to change notification settings - Fork 899
Make Pull and Fetch commands #1288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Maybe put them in extension methods of Network? The class approach would kinda work if you would make the arguments accessible: private readonly Repository repository;
private readonly Signature merger;
private readonly PullOptions options; |
@@ -22,7 +22,7 @@ public void CanFetchIntoAnEmptyRepository(string url) | |||
|
|||
using (var repo = new Repository(path)) | |||
{ | |||
Remote remote = repo.Network.Remotes.Add(remoteName, url); | |||
repo.Network.Remotes.Add(remoteName, url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this cause an issue, since Remote
is now IDisposable
?
I know this is a test method, but we should show the _right thing_™
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in the same situation as before. We dispose of the remotes on repository disposal.
Making this extension methods is the opposite direction to what this is trying to achieve. It's an attempt at making clear what is an operation and what is a method that's trying to replicate what you'd call on the command line. |
This makes it clearer that what we have there is a command and not a method fundamental to the Git system.
6af5806
to
7520d62
Compare
7520d62
to
53dc1c2
Compare
Since we make no use of these commands being classes. Make them static methods of a static class to give them some namespacing.
Yep. I like this.
Nah. Overkill, I think. |
LET THE COMMANDIFICATION BEGIN. |
These want to behave like commands, so let's put them in a namespace that says as much.
This is similar to #1248 but I'm touching the network area so I wanted to send this one as well, so we can discuss the design with more data points.
In particular, should these be classes? They don't really do anything that needs a class. A static method in a static class might work out better for keeping namespacing but not forcing undue instantiations.
These commands take a string instead of having
Remote
andstring
variants since that's what a command would take and all theRemote
instances were doing was carry the name.