-
Notifications
You must be signed in to change notification settings - Fork 711
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
Applied Refactoring #1638
Applied Refactoring #1638
Conversation
@@ -190,4 +192,16 @@ public void append(StringBuffer buffer, String fieldName, Object value, Boolean | |||
super.append(buffer, fieldName, value, fullDetail); | |||
} | |||
}; | |||
|
|||
public String getApiRoute(GHRepository owner) { |
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.
public String getApiRoute(GHRepository owner) { | |
protected final String getApiRoute(GHRepository owner) { |
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.
Generally approve of these refactors. Some questions/requests.
public String getApiTail(String resourceType){ | ||
switch (resourceType) { | ||
case "commits": | ||
return String.format("/repos/%s/%s/git/commits", getOwnerName(), getName()); | ||
case "blobs": | ||
return String.format("/repos/%s/%s/git/blobs", getOwnerName(), getName()); | ||
case "comments": | ||
return String.format("/repos/%s/%s/comments/%s", getOwnerName(), getName(), getId()); | ||
case "trees": | ||
return String.format("/repos/%s/%s/git/trees", getOwnerName(), getName()); | ||
} |
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.
The makes GHRepository
have to know about paths for other resource types. I think this information should be in the classes for those resource types, not here. Extending this out will result in GHRepository
have these string for most other resources.
But I'm open to more discussion if you feel strongly about this.
return StringUtils.prependIfMissing(url.toString().replace(root().getApiUrl(), ""), "/"); | ||
} | ||
@Override | ||
protected String getApiRouteImpl() { |
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.
I don't love this *Impl
pattern, but it makes sense here.
Need a different approach for this api |
Description
Before submitting a PR:
mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.main
. You will create your PR from that branch.When creating a PR: