-
Notifications
You must be signed in to change notification settings - Fork 410
Update create_deployment() #640
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
Update create_deployment() #640
Conversation
|
You'll need to rebase this on top of develop once #652 merges. |
ae408ef to
f9feeae
Compare
|
Done |
|
Looks like flake8 (i.e line too long) failed on the integration test. |
|
Now it's a docstring issue. I'm not quite understanding the error. |
github3/repos/repo.py
Outdated
| tag, or sha. | ||
| :param bool force: Optional parameter to bypass any ahead/behind | ||
| checks or commit status checks. Default: False | ||
| :param list required_contexts Optional array of status contexts |
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 docs job is failing because you need this to be
:param list required_contexts: Optional array ...Not
:param list required_contexts Original array(Notice the : after required_contexts)
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.
Thank you. Fixed in d07683a
github3/repos/repo.py
Outdated
|
|
||
| @requires_auth | ||
| def create_deployment(self, ref, force=False, payload='', | ||
| def create_deployment(self, ref, required_contexts=[], payload='', |
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.
Even though we do no munging of parameter values, I would rather use None and then coerce it to an empty list inside the method. Using mutable default parameters is dangerous if we ever update this to munge required_contexts.
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 made the change in 75a264e.
Is that correct?
|
Perfect! 👍 |
according to https://developer.github.com/v3/repos/deployments/#create-a-deployment
I tried to update the tests for this, but didn't manage to. Any help or feedback would be appreciated.