Skip to content
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 GitHub Reactions support #193

Merged

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented May 28, 2020

This adds the following cmdlets:

The Get and Set can pipe into Remove
Get-GitHubIssue can pipe into Get and Set-GitHubReaction

I also added some tests.

NOTE: commit comments will come later.

NOTE: DO NOT REMOVE THESE REACTIONS THEY ARE USED IN A TEST:

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for starting this effort!

I haven't had a chance to review the tests yet. I'll do so shortly. In the meantime, I have some comments for you to review and address in the code.

Nice job with adding in pipeline support! I'm still mulling over the idea of PassThru and the implications that would have for the rest of the module.

GitHubReactions.ps1 Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
@HowardWolosky HowardWolosky added enhancement An issue or pull request introducing new functionality to the project. api-reactions Work to complete the API's defined here: https://developer.github.com/v3/reactions labels Jun 2, 2020
@HowardWolosky
Copy link
Member

Thanks -- will give a final pass at this tomorrow, and then hopefully get it merged in.

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor nits, then ready to go. Will kick off CI in the meantime.

Tests/GitHubReactions.Tests.ps1 Show resolved Hide resolved
Tests/GitHubReactions.Tests.ps1 Outdated Show resolved Hide resolved
@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

Static analysis failed:

##[warning]GitHubReactions.ps1(79,18): Warning : Command accepts pipeline input but has not defined a process block.
##[warning]GitHubReactions.ps1(84,17): Warning : Command accepts pipeline input but has not defined a process block.
##[warning]GitHubReactions.ps1(203,18): Warning : Command accepts pipeline input but has not defined a process block.
##[warning]GitHubReactions.ps1(208,17): Warning : Command accepts pipeline input but has not defined a process block.
##[warning]GitHubReactions.ps1(203,18): Warning : The parameter 'Uri' has been declared but not used. 
##[warning]GitHubReactions.ps1(216,18): Warning : The parameter 'NoStatus' has been declared but not used. 
##[warning]GitHubReactions.ps1(319,18): Warning : The parameter 'Uri' has been declared but not used. 
##[warning]GitHubReactions.ps1(330,18): Warning : The parameter 'NoStatus' has been declared but not used. 

@TylerLeonhardt
Copy link
Member Author

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 193 in repo microsoft/PowerShellForGitHub

@HowardWolosky
Copy link
Member

Now that #242 is in, we have a standard way for handling the pipeline throughout the module. Are you willing to update this PR to adopt it? Thanks.

@HowardWolosky HowardWolosky added api completeness This is basic API functionality that hasn't been implemented yet. waiting for update Waiting for an update to the PR before the next review and removed enhancement An issue or pull request introducing new functionality to the project. labels Jun 18, 2020
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update and getting this aligned with the rest of the module's pipeline support after #242. Looks like there just a few more updates to make before this is ready to go in.

GitHubReactions.ps1 Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
GitHubReactions.ps1 Show resolved Hide resolved
GitHubReactions.ps1 Show resolved Hide resolved
GitHubReactions.ps1 Outdated Show resolved Hide resolved
Tests/GitHubReactions.Tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubReactions.Tests.ps1 Outdated Show resolved Hide resolved
@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky added waiting for update Waiting for an update to the PR before the next review and removed waiting for update Waiting for an update to the PR before the next review labels Jun 26, 2020
@TylerLeonhardt
Copy link
Member Author

any thoughts?

@HowardWolosky
Copy link
Member

any thoughts?

I was waiting for you to post an update removing the comments support just for this PR so that we could get it in. Then we can work through a good design to add back the comments support. That keeps a good code flow going without having this PR specifically grow and grow and grow.

@TylerLeonhardt
Copy link
Member Author

removed

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TylerLeonhardt
Copy link
Member Author

@HowardWolosky I don't think I impacted these failures

@HowardWolosky HowardWolosky removed the waiting for update Waiting for an update to the PR before the next review label Jul 16, 2020
@HowardWolosky
Copy link
Member

@HowardWolosky Howard Wolosky FTE I don't think I impacted these failures

Agreed -- there's some instability in a couple of the Repository tests that need to be investigated, but those are not considered blocking for getting this in. This should get merged today, and then we can work on adding-in comment support.

# HelpInfo URI of this module
# HelpInfoURI = ''
}
# Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TylerLeonhardt - Can you take a quick look at this file? It looks like the whole file changed. Likely an encoding issue. Would be great if you could get it updated in the PR such that it reflects just the changed lines. Might require you to revert the file and then re-apply the changes. I've noticed that using the GitHub web resolver does this to the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks for this!

@HowardWolosky HowardWolosky merged commit 8e55f5a into microsoft:master Jul 16, 2020
@TylerLeonhardt TylerLeonhardt deleted the added-github-reactions-support branch July 16, 2020 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api completeness This is basic API functionality that hasn't been implemented yet. api-reactions Work to complete the API's defined here: https://developer.github.com/v3/reactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants