Add alias for creating GitHub Releases #7
Conversation
…them separate parameters of GitHubCreateReleaseAsync()
…nnection Reuse the existing logic in GitHubAliases to instantiate the GitHub client. Remove IGitHubClientFactory and GitHubClientFactory which are no longer used.
ap0llo
changed the title
When building on Visual Studio 2017, the xunit analyzer seems to run into an error when checking the siganture of a test case provider. Disabling nullable fpr that method should fix the problem
|
Not sure why I missed this one. Will review asap. |
There was a problem hiding this comment.
First of all, sorry for my late review, the notification got buried in my inbox. This code looks good, I've reviewed most of it and added some comments. Looking forward to hearing your opinion about it.
Once I know what direction we are going with the current comments, we can finalize the review and merge this.
| internal static class MoqExtensions | ||
| { | ||
| public static IReturnsResult<TMock> ThrowsNotFoundAsync<TMock, TResult>(this IReturns<TMock, Task<TResult>> mock) where TMock : class => | ||
| mock.ThrowsAsync(new NotFoundException("", HttpStatusCode.NotFound)); |
There was a problem hiding this comment.
This is not wrong, but I prefer string.Empty
There was a problem hiding this comment.
Sure, I've updated it to use string.Empty
| { | ||
| public class RepositoriesClientMock | ||
| { | ||
| private readonly Mock<IRepositoriesClient> m_Mock = new Mock<IRepositoriesClient>(MockBehavior.Strict); |
There was a problem hiding this comment.
I think we don't use the m_ prefix anywhere else?
There was a problem hiding this comment.
That convention is used in the code bases I usually work on, so I just used it without thinking about it, I've updated all fields to use just a _ prefix
| /// </summary> | ||
| internal class XunitCakeLog : ICakeLog | ||
| { | ||
| private readonly ITestOutputHelper m_TestOutputHelper; |
There was a problem hiding this comment.
_testOutputHelper instead?
| [CakeMethodAlias] | ||
| public static async Task<GitHubRelease> GitHubCreateReleaseAsync( | ||
| this ICakeContext context, | ||
| string userName, |
There was a problem hiding this comment.
should this be nullable, similar to what is mentioned in the docs?
| /// </summary> | ||
| public sealed class GitHubRelease | ||
| { | ||
| private readonly List<GitHubReleaseAsset> m_Assets = new List<GitHubReleaseAsset>(); |
There was a problem hiding this comment.
use _assets instead?
|
|
||
| internal void Add(GitHubReleaseAsset asset) | ||
| { | ||
| if (asset is null) |
There was a problem hiding this comment.
Can this be null when we have nullable enabled on the project?
There was a problem hiding this comment.
You're right, we can omit the null checks for internal methods, I've updated the code accordingly
| /// </summary> | ||
| internal static GitHubReleaseAsset FromReleaseAsset(ReleaseAsset asset) | ||
| { | ||
| if (asset is null) |
No worries :)
All your comments are reasonable, I've updated the code accordingly. Would you like me to squash the commits together a bit or should I just leave the cleanup commits in place? |
My preference is to keep it, even for PRs the history of commits can be very useful to understand the "thought process" afterwards. So if you agree, let's keep it. Will do a re-review as soon as I can. |
| /// </summary> | ||
| internal static GitHubRelease FromRelease(Release release) | ||
| { | ||
| if (release is null) |
|
|
||
| public GitHubReleaseCreator(ICakeLog cakeLog, IFileSystem fileSystem, IGitHubClient githubClient) | ||
| { | ||
| _cakeLog = cakeLog ?? throw new ArgumentNullException(nameof(cakeLog)); |
There was a problem hiding this comment.
the throws can be removed as well, nothing will ever be null here.
| if (String.IsNullOrWhiteSpace(tagName)) | ||
| throw new ArgumentException("Value must not be null or whitespace", nameof(tagName)); | ||
|
|
||
| if (settings is null) |
| var existingRelease = await TryGetReleaseAsync(owner: owner, repository: repository, tagName: tagName); | ||
| if (existingRelease != null) | ||
| { | ||
| if (settings.Overwrite) |
There was a problem hiding this comment.
just for readability: if we invert this, it saves us reading / understanding the code:
if (!settings.Overwrite)
{
throw new Release ....
}
await DeleteReleaseAsync...
b928545 to
8122a62
Compare
|
@pascalberger we might need your help here. @ap0llo did a great job adding this PR, and I've reviewed (and approved). Do we need to synchronize any other files to make this work (e.g. cake scripts, recipes, etc)? |
What changes are you thinking need to happen here? |
|
I saw the check failing, and it's been a while we synced the recipe files, so I made the assumption it might be time for a synchronization. Can we just merge this PR? |
|
Oh, I see what you mean... There is a known issue that I have seen with running GitVersion on AppVeyor when it is a Pull Request. I am not entirely sure what is going on there, as I haven't dug into it. It could be that this is solved with a newer version of GitVersion, but as mentioned, I haven't dug into it. If the build is working locally for you, then I would recommend merging the PR, and in all likelihood it will succeed currently when building off the develop branch. |
|
Thanks for the explanation, going to merge now. |
Description
This PR adds the alias
GitHubCreateReleaseAsync()which allows creating a GitHub Release from cake,as discussed in ap0llo/Cake.GitHubReleases#1.
Since the code I brought over from ap0llo/Cake.GitHubReleases was already using nullable reference types, I enabled nullable reference types (and C# 8) for the entire project and added nullable annotations to the existing code.
Related Issue
Motivation and Context
With this change, it will be possible to create a GitHub Release from a Cake script.
The alias simplifies creation of the release without the need to use Octokit directly.
Integrating this functionality into Cake.GitHub rather than maintaining a separate addin should make it easier for users to integrate GitHub functionality into their Cake script and also avoids potential dependency conflicts that might arise from two addins depending on different versions of Octokit.
How Has This Been Tested?
Almost all of the functionality for creating releases is implemented in the
GitHubReleaseCreatorclass.I added unit tests to the existing
Cake.GitHub.Testsproject that cover all scenarios (at least all of the scenarios I could think of). The tests use Moq to mock the access to GitHub.Types of changes
Checklist:
I tried to make the signature of
GitHubCreateReleaseAsync()as similar to the existingGitHubStatus()alias to make both of them as consistent as possible. If you have additional notes w.r.t. code style, please let me know