-
Notifications
You must be signed in to change notification settings - Fork 736
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
#218 enable cross fork compare #219
Conversation
Kohsuke Kawaguchi » github-api #394 SUCCESS |
fqid1 = String.format("%s:%s",owner1,id1.getName()); | ||
fqid2 = String.format("%s:%s",owner2,id2.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.
catch should be after } -> } catch {
👎 For now. I think it should be:
|
Kohsuke Kawaguchi » github-api #395 SUCCESS |
@@ -682,7 +682,24 @@ public GHCompare getCompare(GHCommit id1, GHCommit id2) throws IOException { | |||
} | |||
|
|||
public GHCompare getCompare(GHBranch id1, GHBranch id2) 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.
please avoid empty lines in count more than one
Kohsuke Kawaguchi » github-api #396 SUCCESS |
String ownerName1 = owner1.getOwnerName(); | ||
String ownerName2 = owner2.getOwnerName(); | ||
if (!StringUtils.equals(ownerName1, ownerName2)) { | ||
String qualifiedName1 = String.format("%s:%s",ownerName1,id1.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.
please add space after ,
👍 after some code style fixes |
Kohsuke Kawaguchi » github-api #397 SUCCESS |
As I see from the code, null owners are not supposed to happen. BTW I'm not sure I've investigated all potential cases. 👍 in any case, because the original behavior smells like a bug |
this addresses #218