8000 Update create_deployment() by adrianmoisey · Pull Request #640 · sigmavirus24/github3.py · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@adrianmoisey
Copy link
Contributor

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.

@sigmavirus24
Copy link
Owner

You'll need to rebase this on top of develop once #652 merges.

@adrianmoisey
Copy link
Contributor Author

Done

@itsmemattchung
Copy link
Contributor

Looks like flake8 (i.e line too long) failed on the integration test.

@adrianmoisey
Copy link
Contributor Author

Now it's a docstring issue. I'm not quite understanding the error.

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
Copy link
Owner
@sigmavirus24 sigmavirus24 Nov 23, 2016

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)

Copy link
Contributor Author

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


@requires_auth
def create_deployment(self, ref, force=False, payload='',
def create_deployment(self, ref, required_contexts=[], payload='',
Copy link
Owner

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.

Copy link
Contributor Author

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?

@sigmavirus24
Copy link
Owner

Perfect! 👍

@sigmavirus24 sigmavirus24 merged commit 06bf42c into sigmavirus24:develop Nov 23, 2016
@adrianmoisey adrianmoisey deleted the deployment_api_update branch November 23, 2016 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0