Fix build regressions, runtime NPE, and clean up control flow - #147
#147Fix build regressions, runtime NPE, and clean up control flow#147scurth wants to merge 13 commits into
Conversation
| logger.info(" tiling isometric base tile."); | ||
| logger.info(""); | ||
| logger.info("Two results are produced by this tool: \"generated.png\" and \"generated-rotated.png\""); | ||
| logger.info("The best one to use depends on the texture."); |
There was a problem hiding this comment.
This is a command-line util - so writing the usage help to the log is wrong. I'm guessing you have applied suggested fixes from the IDE without verifying.
| @@ -107,7 +107,7 @@ public ChangeSet ready(ServerPlayer serverPlayer, boolean ready) { | ||
| * @return A {@code ChangeSet} encapsulating this action. | ||
| */ | ||
| public ChangeSet requestLaunch(ServerPlayer serverPlayer) { | ||
| logger.info("Launching for " + serverPlayer); | ||
| if (logger.isLoggable(Level.INFO)) logger.info("Launching for " + serverPlayer); |
There was a problem hiding this comment.
If-tests like these are often suggested by tools like PMD, but is more complicated to read - and for what?The cost of the string construction is insignificant.
|
First of all, it's really nice that you want to contribute to the FreeCol code! There are too many changes that are unrelated in the same Pull request (PR). You need to create separate PRs for each type of change. In addition, you need to state how your changes have been tested. There are a lot of good changes in the PR, but it's simply too large and includes changes we do not want to merge. Please do make separate PRs if you have the time to do so. |
|
Thanks a lot for checking this, indeed way too large a PR. I used my I just created a well-scoped PoC and submitted PR #172 to showcase the |
Changes