-
Notifications
You must be signed in to change notification settings - Fork 724
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
add create repo with template support #897
add create repo with template support #897
Conversation
Signed-off-by: Yang Ting <bonnie.young@maxwit.com>
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.
@bonnie-young
Thanks for the PR. High quality and with tests. Awesome!
Because this is adding new functionality, I need to look at it a bit more. I also don't know enough about repository template API to land this just yet. I'll look at this in the next few days.
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.
@bonnie-young
This is good but I'll like to see it look a little different. I've added some suggestions as examples but don't treat those as what it has to look like.
If you feel strongly opposed to anything I'm suggesting please say so and we can discuss. I value all view points in the continuing growth and improvement of this project.
Thanks for contributing.
@@ -20,6 +22,22 @@ | |||
this.builder.with("name", name); | |||
} | |||
|
|||
GHCreateRepositoryBuilder(GitHub root, String apiUrlTail, String name, Boolean isTemplate) { |
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.
@bonnie-young Could you tell me why we need two new constructors?
Could we add an templateRepository(boolean)
method to the builder instead?
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.
yes, you are right! sorry for my carelessness.
* @throws IOException | ||
* if repsitory cannot be created | ||
*/ | ||
public GHRepository createWithTemplate() throws IOException { |
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.
Similar to my previous comment could we have a fromTemplateRepository(String templateOwner, String templateRepo)
method? I think the create()
method could have .withPreview(BAPTISE)
set in general without any negative effect, but I could be wrong.
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.
According to the official doc , I don't think we can add an fromTemplateRepository(String templateOwner, String templateRepo)
to the builder, Because templateOwner and templateRepo are parts of the request url not parameters. anyway, I will try it out, and find the result.
"create() method could have .withPreview(BAPTISE) set in general without any negative effect" —— I will try it later also, thanks for your idea.
* the organization of repository to be created | ||
* @return the gh create repository builder | ||
*/ | ||
public GHCreateRepositoryBuilder createRepositoryWithTemplate(String templateRepo, |
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.
Ah, I see. The create from template repository has almost none of the parameters of general repository creation.
Still I think that limitation is something that could be explained in the javadoc for fromTemplateRepository(String templateOwner, String templateRepo)
and include a link to the GitHub docs.
What you've done is good: guiding users to the a non-failure path. However, in this case, I'd rather reduce the surface area and added code in this library.
Also add @Preview
and @Deprecated
annotations to the methods that call GitHub Preview APIs.
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.
“Also add @Preview and @deprecated annotations to the methods that call GitHub Preview APIs.” —— I see. Thank you for reminding me. But I don't know why need to add the @deprecated annotation.
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.
“Also add @Preview and @deprecated annotations to the methods that call GitHub Preview APIs.” —— I see. Thank you for reminding me. But I don't know why need to add the @deprecated annotation.
I found the answer from oracle official doc:
When to Deprecate
When you design an API, carefully consider whether it supersedes an old API. If it does, and you wish to encourage developers (users of the API) to migrate to the new API, then deprecate the old API. Valid reasons to deprecate an API include:
- It is insecure, buggy, or highly inefficient (I got this point)
- It is going away in a future release
- It encourages bad coding practices
@bonnie-young |
@bitwiseman |
@bonnie-young I plan to publish the next version soon, but we can wait a few days to get this last addition in. |
@bitwiseman |
a2bc5c6
to
5e703be
Compare
a3181a0
to
6afd248
Compare
Signed-off-by: Yang Ting <bonnie.young@maxwit.com>
6afd248
to
11bc669
Compare
Signed-off-by: Yang Ting bonnie.young@maxwit.com
Description
Before submitting a PR:
We love getting PRs, but we hate asking people for the same basic changes every time.
master
. Create your PR from that branch.mvn clean compile
locally. This may reformat your code, commit those changes.mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.When creating a PR: