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

Create from template #2012

Closed
wants to merge 27 commits into from
Closed

Conversation

simkimsia
Copy link
Contributor

This is to solve #1395 and #1974

@simkimsia
Copy link
Contributor Author

What else do I need to change to have this approved?

@Archweii
Copy link

Archweii commented Sep 3, 2021

I'm wondering how i can help to make this PR pass, i'm not a contributor but i really need this feature for a script im doing at work.

From what i have seen in the API reference (https://docs.github.com/en/rest/reference/repos#create-a-repository-using-a-template), the special header is not longer required.

Copy link
Collaborator

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

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

Looking like a good start, but I have some concerns firstly inline, but this these methods look very identical, I wonder if we could ease that duplication. This is also missing type hints for the new methods.

self._requester, headers, data, completed=True
)

def create_repo_from_template(
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 duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 6ddabf7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also missing type hints for the new methods.

NOt too sure what this means. Can advise after my commit 6ddabf7

post_parameters["private"] = private
headers, data = self._requester.requestJsonAndCheck(
"POST",
"/repos/" + repo.owner.login + "/" + repo.name + "/generate",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use an f-string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved by 6ddabf7

repo.url,
"https://api.github.com/repos/jacquev6/hello-world-docker-action-new",
)
self.assertEqual(repo.is_template, False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.assertFalse()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved by commit 6ddabf7

@harismuha123
Copy link

@simkimsia any news on when you might perform these changes requested so this can be merged in? This is also needed for a project I'm working on.

@simkimsia
Copy link
Contributor Author

simkimsia commented Oct 21, 2021

For some reason the commit I made to merge resolve conflicts is not reflecting in this PR on GitHub so I will close this PR and re-create a new PR

@harismuha123 please look at #2088 instead.

@s-t-e-v-e-n-k Sorry I had to close this PR to make sure the latest merge commit 652bdcc that addresses the merge conflict shows up in the PR, so please continue the review over at #2088.

Thank you

@simkimsia simkimsia closed this Oct 21, 2021
This was referenced Oct 21, 2021
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

5 participants