chore: fix issues with compilation on later JDKs #178
Conversation
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
from
bfb8d8c to
120b127
Compare
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
from
120b127 to
5ef9d03
Compare
Codecov Report
@@ Coverage Diff @@
## main #178 +/- ##
=========================================
Coverage 91.60% 91.60%
Complexity 187 187
=========================================
Files 20 20
Lines 417 417
Branches 22 22
=========================================
Hits 382 382
Misses 23 23
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
from
5ef9d03 to
967ea50
Compare
toddbaert
changed the title
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
from
967ea50 to
80f80d5
Compare
There was a problem hiding this comment.
If you're somehow blocked on jdk versions.. I'd like to separate that from the documentation changes. I think the documentation changes are of low value (no offense intended) and I don't really want to accept them. No one needs to know that collection.add(thing) adds the thing to the collection. It's apparent on it's face.
| @Override | ||
| public Value getValue(String key) { | ||
| return this.attributes.get(key); | ||
| } | ||
|
|
||
| // adders | ||
| /** | ||
| * Adds the specified value at key. |
There was a problem hiding this comment.
These docs feel kinda useless?
There was a problem hiding this comment.
If you're somehow blocked on jdk versions
I'm not blocked... I just find it tedious, and the effort to build with 8+ is relatively low.
If you're somehow blocked on jdk versions.. I'd like to separate that from the documentation changes. I think the documentation changes are of low value (no offense intended) and I don't really want to accept them. No one needs to know that collection.add(thing) adds the thing to the collection. It's apparent on it's face.
I see your point. While these ones in particular aren't super valuable, even in this PR there are some that are (I think some of the exceptions are nice, for instance).
I guess the questions is, would you rather see us add this rule, and ignore it where we think it's silly, or not add the rule, and be careful to add doc where users might be confused.
Chose what you prefer and I will update accordingly.
There was a problem hiding this comment.
My preference would be to enable the rule and explicitly say "skip this one" on the things that are very obvious to keep the tool happy.
There was a problem hiding this comment.
Hey @justinabrahms I've done this.
I removed the lowest value javadocs (like the ones for the adders) and added the capability to suppress checkstyle warnings with annotations for those instances.
toddbaert
changed the title
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
from
80f80d5 to
b76807a
Compare
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
5 times, most recently
from
05149f1 to
1ccfd5f
Compare
| <!-- https://checkstyle.org/config_filters.html#SuppressionXpathFilter --> | ||
| <module name="SuppressionXpathFilter"> | ||
| <property name="file" value="${org.checkstyle.google.suppressionxpathfilter.config}" | ||
| default="checkstyle-xpath-suppressions.xml" /> | ||
| <property name="optional" value="true"/> | ||
| </module> |
There was a problem hiding this comment.
I moved all the suppression-related config to the top of the parent element.
| <module name="SuppressWarningsFilter" /> | ||
|
|
||
| <module name="TreeWalker"> | ||
| <!-- needed for SuppressWarningsFilter --> | ||
| <module name="SuppressWarningsHolder" /> | ||
|
|
||
| <module name="SuppressWarnings"> | ||
| <property name="id" value="checkstyle:suppresswarnings"/> | ||
| </module> |
There was a problem hiding this comment.
New stuff to support suppression via annotations.
| /** | ||
| * Predefined resolution reasons. | ||
| */ |
There was a problem hiding this comment.
This one it debatable but I wanted to make it clear that you're not limited to just these reasons, these are just some we pre-defined.
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
from
1ccfd5f to
188e795
Compare
Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
chore/fix-jdk-11+
branch
from
188e795 to
cd0efb7
Compare
This just fixes an issue that prevents JDK 11+ from working with our build and adds some associated missing javadoc. This is troublesome if you happen to be working with other projects that require a later version of the JDK.
Specifically, there were changes in later versions of
javacthat applied more rules, mostly around javadoc. These caused errors we could not customize, so I've disabled the lint checks in themaven-javadoc-pluginand I instead enforce them withcheckstyle, which is more customizable.