Refactored Network.Fetch() with FetchOptions #584
Conversation
| /// </summary> | ||
| /// <param name="repository">The <see cref="Repository"/> being worked with.</param> | ||
| /// <param name="remoteName">The name of the <see cref="Remote"/> to fetch from.</param> | ||
| public static void Fetch(this IRepository repository, string remoteName) |
There was a problem hiding this comment.
How about dropping this overload...
There was a problem hiding this comment.
...And replacing the call in FetchHeadFixture.cs with the following?
repo.Network.Fetch(repo.Network.Remotes["origin"]);There was a problem hiding this comment.
Don't you think that some production code is using Repository.Fetch(string)? We would break that without notice.
We would have to keep this hack in Network.Fetch(Remote) anyway, otherwise that statement would now work, either. In the end, when the obsolete overloads are removed, we can drop them both.
There was a problem hiding this comment.
Could you update the "obsoleted" method to call the updated API (e.g. create the FetchOptions object from the input parameters)? That way you would not need a separate 2 parameter method.
There was a problem hiding this comment.
That does not help. A call to Fetch(string) would still be ambiguous, because there is a Fetch(string, [TagFetchMode], ...) and a Fetch(string, [FetchOptions]) overload. The compiler does not know which one to pick, because both accept a single string argument, and neither of them gets precedence. Only a Fetch(string) overload is favored over these two, and eliminates the error.
There was a problem hiding this comment.
Duh. Let's keep it that way, then.
Changed the signature to Network.Fetch(Remote, FetchOptions). As of libgit2#536, optional parameters should be grouped in a xxxOptions type. Network.Fetch() has currently 5 optional parameters, and a sixth one (RefSpecs) will be introduced soon.
|
💯 ✨ |
Changed the signature to
Network.Fetch(Remote, FetchOptions).As of #536,
Network.Fetch()has currently 5 optional parameters, and a sixth one (RefSpecs) will be introduced in #572. I think aFetchOptionsclass is by far more clear.I'm not sure if we should keep tests for the obsolete syntax.