-
Notifications
You must be signed in to change notification settings - Fork 254
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
Update to jdk9 and latest dependency versions #120
Conversation
Java8: - enable checker framework checks Java9: - disable checker framework checks - configure cucumber to avoid certain warnings
To do: See if other categories containing meaningful rules. Then remove commented out obsolete rule sets.
Warnings can be suppressed through comments or annotations. Simplify configuration by allowing annotations only (which are also clearer, as they come with a reason). Change to this configuration needed anyway the comment filters were moved in checkstyle 8.6.
Enforce maven 3.1.1 as needed by the spotbugs plugin.
No default manifest, no need to specify jar besides test jar.
@@ -23,7 +23,7 @@ | |||
*/ | |||
Board(Square[][] grid) { | |||
assert grid != null; | |||
this.board = grid; | |||
this.board = (Square[][]) grid.clone(); |
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 are we using clone and a cast 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.
Well spotted ;-).
This relates to the PMD rule Array is stored directly.
The deeper issue is that we should rethink the pmd rule set, conform #121.
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 think this will run into more problems than it would solve. If we do want to address it, we must make sure that we can actually copy a Square properly. That said, if we need to keep this in to keep PMD happy let's add a TODO to make sure we address this as soon as possible (once #121 is resolved)
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.
So what about Arrays.copy?
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. "Solving" this non-problem would require a deep copy, which Arrays.copyOf also does not do.
My proposal is to undo the clone addition, add a SuppressWarnings here referencing the rule, and then revisit this when we recheck the pmd rule set to be used. (And I'm beginning to be in favor of dropping this rule).
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.
@avandeursen Yes that seems to be an appropriate solution 👍
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.
Pending the reversal of the .clone()
invocation and using a SuppressWarnings
, this PR LGTM
Thanks @mjwsteenbergen @LiamClark @TimvdLippe! |
Fixes #119.