-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add alias for creating GitHub Releases #7
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not wrong, but I prefer string.Empty
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't use the m_
prefix anywhere else?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_testOutputHelper
instead?
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.
Fixed
[CakeMethodAlias] | ||
public static async Task<GitHubRelease> GitHubCreateReleaseAsync( | ||
this ICakeContext context, | ||
string userName, |
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.
should this be nullable, similar to what is mentioned in the docs?
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.
You're right
/// </summary> | ||
public sealed class GitHubRelease | ||
{ | ||
private readonly List<GitHubReleaseAsset> m_Assets = new List<GitHubReleaseAsset>(); |
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.
use _assets
instead?
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.
Fixed
|
||
internal void Add(GitHubReleaseAsset asset) | ||
{ | ||
if (asset is null) |
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.
Can this be null when we have nullable enabled on the project?
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.
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) |
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.
no need to check?
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.
Fixed
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. |
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.
Reviewed everything except the unit tests. Let's discuss the current comments and see if we both agree on them.
Then we can hopefully finalize this PR because it looks great so far 👍
/// </summary> | ||
internal static GitHubRelease FromRelease(Release release) | ||
{ | ||
if (release is null) |
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.
cannot be null
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.
Fixed
|
||
public GitHubReleaseCreator(ICakeLog cakeLog, IFileSystem fileSystem, IGitHubClient githubClient) | ||
{ | ||
_cakeLog = cakeLog ?? throw new ArgumentNullException(nameof(cakeLog)); |
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.
the throws can be removed as well, nothing will ever be null here.
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.
Fixed
if (String.IsNullOrWhiteSpace(tagName)) | ||
throw new ArgumentException("Value must not be null or whitespace", nameof(tagName)); | ||
|
||
if (settings is null) |
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.
this can be removed
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.
Fixed
var existingRelease = await TryGetReleaseAsync(owner: owner, repository: repository, tagName: tagName); | ||
if (existingRelease != null) | ||
{ | ||
if (settings.Overwrite) |
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.
just for readability: if we invert this, it saves us reading / understanding the code:
if (!settings.Overwrite)
{
throw new Release ....
}
await DeleteReleaseAsync...
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.
Fixed
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
GitHubReleaseCreator
class.I added unit tests to the existing
Cake.GitHub.Tests
project 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