-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
feature-send private message to user #9
Conversation
@@ -25,14 +30,22 @@ | |||
#Set whether or not this message should trigger a notification for people in the room. (default: false) | |||
[switch]$notify, | |||
|
|||
#Required. Server name, default to 'api.hipchat.com' | |||
[Parameter(Mandatory = $True)] |
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't be mandatory = true and have a default. Suggest you remove the 'mandatory = true'.
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 fully agree.
This would normally be tested in the CI; but this is not yet implemented in the project.
I created #12 to fix this.
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.
Actually there is CI already on this project, it runs from my AppVeyor account still. The tests are currently failing because -server
is mandatory and not explicitly declared in the tests.
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.
ok... I meant:
The CI and the tests are not yet aligned with the rest of the projects and the branches to not forbid merging while the CIs do not pass.
sorry for merging without checking the CI
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 it
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.
Ah I see, thanks for the clarification :)
Description
divy up API methods using ParameterSetName
add another API namespace
add var server for private servers
Motivation and Context
closes #8
Types of changes
Checklist: