-
Notifications
You must be signed in to change notification settings - Fork 735
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
Cleanup issues discovered by FindBugs #210
Conversation
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
} | ||
|
||
public GHContentUpdateResponse createContent(String content, String commitMessage, String path, String branch) throws IOException { | ||
return createContent(content.getBytes(), commitMessage, path, branch); | ||
//TODO: UTF-8? |
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.
yep, plz change to utf8
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.
Is it a standard for GitHub?
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. Content-type includes utf-8 as charset
Kohsuke Kawaguchi » github-api #380 SUCCESS |
@@ -128,7 +129,9 @@ | |||
} else { | |||
if (password!=null) { | |||
String authorization = (login + ':' + password); | |||
encodedAuthorization = "Basic "+new String(Base64.encodeBase64(authorization.getBytes())); | |||
//TODO: UTF-8? | |||
final Charset charset = Charset.defaultCharset(); |
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.
@lanwen the same?
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.
yep
👍 |
@@ -942,7 +949,7 @@ private void verifyMine() throws IOException { | |||
if (!root.login.equals(owner.login)) | |||
throw new IOException("Operation not applicable to a repository owned by someone else: "+owner.login); | |||
} | |||
|
|||
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.
typo?
Kohsuke Kawaguchi » github-api #381 SUCCESS |
👍 and squash before merge please |
88ca014
to
79c5b2e
Compare
@KostyaSha |
Kohsuke Kawaguchi » github-api #382 SUCCESS |
👍 |
Cleanup issues discovered by FindBugs
@@ -180,7 +180,8 @@ GHThread fetch() { | |||
private long calcNextCheckTime() { | |||
String v = req.getResponseHeader("X-Poll-Interval"); | |||
if (v==null) v="60"; | |||
return System.currentTimeMillis()+Integer.parseInt(v)*1000; | |||
long seconds = Integer.parseInt(v); |
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.
Wouldn't Long.parseLong()
suit better here?
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.
Depends on what is in X-Poll-Interval
but you may be right
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.
This is the same, I just noticed it.
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.
In any case this code does not handle parsing exceptions, which may strike back at some point
The change closes about 100 issues (mostly API ones), but there're also several changes for encodings and file streams
@reviewbybees @lanwen @KostyaSha