Implementations of the missing RepositoriesClient methods #319
Conversation
|
I think |
|
I guess that's what I get for copying existing comments and then modifying them. Forgot to remove the first "the". And apparently I did it for all of the comments that have the same format. I'll sort that out after work. |
|
Or maybe on the lunch break, because waiting until after work is just too long. |
| using System.Threading.Tasks; | ||
| using Octokit; | ||
| using Octokit.Tests.Integration; | ||
| using Xunit; | ||
| using System; |
|
Regarding AssertEx.DoesNotAllowNullArguments and DoesNotAllowEmptyStringArguments. |
Let's address those separately - personally I don't mind seeing the |
Added unit tests for newly added methods Added models RepositoryTag, RepositoryLanguage and RepositoryUpdate
sitoriesClient Added unit tests for new methods.
RepositoriesClient
|
Removed the assertion methods and cleaned up the commits. |
|
|
||
| var contributors = await github.Repository.GetAllContributors("octokit", "octokit.net"); | ||
|
|
||
| Assert.True(contributors.Any(c => c.Login == "pmacn")); |
There was a problem hiding this comment.
Indulging in a little bit of self gratification
|
Ok, I think this one is good to go. Merge master in and 💥 |
Conflicts: Octokit.Reactive/Clients/IObservableRepositoriesClient.cs Octokit.Reactive/Clients/ObservableRepositoriesClient.cs Octokit/Helpers/ApiUrls.cs Octokit/Octokit-Mono.csproj Octokit/Octokit-MonoAndroid.csproj Octokit/Octokit-Monotouch.csproj Octokit/Octokit-netcore45.csproj
|
@shiftkey merged~ |
|
I did just realize that I didn't add a |
|
@shiftkey Should be all done now |
Implementations of the missing RepositoriesClient methods
There are some concerns regarding naming here to be aware of. Specifically the response model
RepositoryTag. A class namedGitTagalready exists but the structures are quite different. It looks as if this might be more in line with a lightweight tag soLightweightTagmight do as a name instead.Secondly the builder implementation in the integration tests is silly at this point, but it felt even sillier repeating the longer section over and over. And this way the option to change the defaults is more centralized and it's easy to add methods to the builder for more customization if it's ever needed. I can take it or leave it but if it stays I have another commit that replaces the hardcoded instantiations throughout the integration tests.
Hopefully there aren't many other issues :)