Skip to content

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

Merged
merged 3 commits into from
Mar 28, 2016
Merged

Make Pull and Fetch commands #1288

merged 3 commits into from
Mar 28, 2016

Conversation

carlosmn
Copy link
Member

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 and string variants since that's what a command would take and all the Remote instances were doing was carry the name.

@txdv
Copy link
Contributor

txdv commented Mar 22, 2016

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);
Copy link
Member

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_™

Copy link
Member Author

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.

@carlosmn
Copy link
Member Author

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.
Since we make no use of these commands being classes. Make them static
methods of a static class to give them some namespacing.
@ethomson
Copy link
Member

Yep. I like this.

In particular, should these be classes?

Nah. Overkill, I think.

@ethomson
Copy link
Member

LET THE COMMANDIFICATION BEGIN.

@ethomson ethomson merged commit 21c0c15 into master Mar 28, 2016
@carlosmn carlosmn deleted the cmn/pull-command branch April 12, 2016 21:49
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.

4 participants