8000 Add support for default commit message by broady · Pull Request #650 · sigmavirus24/github3.py · GitHub
[go: up one dir, main page]
Skip to content

Add support for default commit message #650

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.

Sign up for GitHub

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

Jump to bottom
Merged
itsmemattchung merged 2 commits into from
Dec 17, 2016
Merged

Add support for default commit message #650

itsmemattchung merged 2 commits into from
Dec 17, 2016

Conversation

@broady
Copy link
Contributor
@broady broady commented Nov 14, 2016

Fixes #649.

@sigmavirus24
Copy link
Owner
sigmavirus24 commented Nov 22, 2016

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

@sigmavirus24 sigmavirus24 reopened this Nov 23, 2016
@sigmavirus24
Copy link
Owner
sigmavirus24 commented Nov 23, 2016

Closing & Reopening apparently will trigger a new merged build on Travis.

@itsmemattchung
Copy link
Contributor
itsmemattchung commented Nov 25, 2016

Thanks for submitting the PR. Instead of modifying the existing test case, test_merge, please add a new test case to cover the new use case (i.e not passing in commit_message)

@broady
Copy link
Contributor Author
broady commented Dec 2, 2016

I haven't forgotten about this. I'll finish this up in the next couple days. Thanks for the review.

@broady
Copy link
Contributor Author
broad 8000 y commented Dec 17, 2016

@itsmemattchung actually, this is a change in behaviour. no new test is needed.

please take another look. i've rebased the change.

@itsmemattchung itsmemattchung merged commit 67f38a2 into sigmavirus24:develop Dec 17, 2016
@itsmemattchung
Copy link
Contributor
itsmemattchung commented Dec 17, 2016

I incorrectly assumed this was an additional feature, based off of the PR title, not a change in behavior. Thanks for clarifying @broady .

From developer docs:
"Commit message to use for the merge commit. If omitted, a default message will be used"

@itsmemattchung actually, this is a change in behaviour. no new test is needed.

PR looks good to me this since this really is a change in behavior. I'm even more comfortable after confirming a test already exists where we pass in commit_message that is not None: test_merge_squash_message

Thanks again @broady .
🍰

@sigmavirus24
Copy link
Owner
sigmavirus24 commented Dec 17, 2016

Thanks @broady !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@broady @sigmavirus24 @itsmemattchung
0