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

repos: Allow to create draft pull requests #969

Closed

Conversation

dbaty
Copy link

@dbaty dbaty commented Sep 8, 2019

Repository.create_pull and create_pull_from_issue now take an
optional draft argument.


This pull request adds support to create draft pull requests. There is already another pull request (#927) to read the draft status when fetching pull requests. These two pull requests are independent and could be merged separately. I just reused the constant pulls.PULLS_PREVIEW_HEADERS that is added by #927.

Integration tests fail: some cassettes should be updated to include the draft parameter that we now send. It's not clear to me how to do that properly. I tried to add record='all' to calls to use_cassette() in those tests, but I get a 403 Must have admin rights to Repository. error on the call to `original_repository.create_fork(organization="testgh3py"). Which I don't find surprising, of course. :) What's the way forward, here?

`Repository.create_pull` and `create_pull_from_issue` now take an
optional `draft` argument.
@omgjlk
Copy link
Collaborator

omgjlk commented Sep 9, 2019

Welcome! Thanks for your contribution. We'll be reviewing the code soon(ish) and leaving a pull request review. Our time to review is somewhat... variable. The joys of volunteer open source development :D

Copy link
Collaborator

@omgjlk omgjlk left a comment

Choose a reason for hiding this comment

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

I think this is a great start. I've left some comments.

The linter is also complaining about line length. You should be able to follow the links from the CI status bots to find where the tests are failing and adjust accordingly.

The other maintainers may have more feedback for you as well.

)
repository.delete()

assert isinstance(pull_request, github3.pulls.ShortPullRequest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a great start here. I think we should be able to validate that the pull request was created in draft mode here as well.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, but that would require your own pull request #927 to be merged first.

@@ -322,9 +322,13 @@ def test_create_pull_private_required_data(self):

def test_create_pull_private(self):
"""Verify the request for creating a pull request."""
data = {"title": "foo", "base": "master", "head": "feature_branch"}
data = {"title": "foo", "base": "master", "head": "feature_branch", "draft": True}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes the test case to only test the draft code path. For proper coverage there should be two tests, one that tests the draft code path and one that tests the non-draft code path.

@sigmavirus24
Copy link
Owner

I'm fairly certain this is present in main

@SergNikitin
Copy link

@sigmavirus24 can you or perhaps somebody else elaborate on how does this currently work in main? To make this unmerged PR obsolete I'd expect repo.create_pull() to have some sort of parameter related to draft status, but it doesn't look like there is one.

It looks like a call to repo._create_pull() should work if used like

repo._create_pull({"title": pr_title, "body": message, "base": "master", "head": head, "draft": True})

But that's a private method I assume that users shouldn't access. Not sure if #926 should've been closed too

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.

None yet

4 participants