-
Notifications
You must be signed in to change notification settings - Fork 29
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
[244] address sonar issues in wildfly/cdk bundles #303
Conversation
@@ -52,7 +52,7 @@ | |||
private static final Logger LOG = LoggerFactory.getLogger(URLTransportCache.class); | |||
|
|||
private static final int DEFAULT_CONNECT_TIMEOUT = 1 * 60 * 1000; | |||
private static final int DEFAULT_READ_TIMEOUT = 2 * 60 * 1000; | |||
private static final int DEFAULT_READ_TIMEOUT = (int)(0.1 * 60 * 1000); |
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 this intentional, to change the read timeout from 60 seconds to 6 seconds?
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.
no wasnt, my bad. Reverting it. I most likely introduced this when I started writing tests and forgot to revert it.
return "CDK"; | ||
} | ||
if( props != null | ||
&& props.getCDKVersion() != null ) { |
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.
couldn't this be on one line? if( props != null && props.getCDKVersion() != null ) {
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 prefer to separate both condition to the max and thus having every condition on a separate line
@@ -55,7 +60,8 @@ public static int getWorkflowStatus(DownloadRuntime dr, String userS, String pas | |||
if (response == 401) { | |||
// 401 means bad credentials, change nothing | |||
return CREDENTIALS_FAILED; | |||
} else if (response == 403 || response == 200) { | |||
} else if (response == 403 | |||
|| response == 200) { |
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.
Why does it move this to a new line? It's not even very long :( I hate automatic code formatters :(
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, too. On purpose. Like you prefer not to immediately return a value but to store in a temporary variable and only return afterwards: I prefer to have every condition on it's own line and get better readable imho.
Codecov Report
@@ Coverage Diff @@
## master #303 +/- ##
===========================================
+ Coverage 31.05% 31.35% +0.3%
- Complexity 1378 1389 +11
===========================================
Files 298 298
Lines 11302 11302
Branches 1590 1593 +3
===========================================
+ Hits 3510 3544 +34
+ Misses 7347 7310 -37
- Partials 445 448 +3
Continue to review full report at Codecov.
|
If you just verify the DEFAULT_READ_TIMEOUT situation, I can +1. That timeout change seems iffy to me though. |
Signed-off-by: Andre Dietisheim <[email protected]>
@robstryker corrected the timeout, please re-review. |
this is part of the effort for issue #244