Issue #6241: resolved teamcity 2018.3 violations #6242

rnveach · 2018-12-02T17:04:27Z

These are the ones I know can be fixed. Most others will need suppressions. I'm not sure Control flow issues - Simplifiable boolean expression (1) is wise because of pitest.

src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java

romani

item to improve:

src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java

romani · 2018-12-02T18:48:11Z

@rnveach , I think it is reasonable to suppress complicated cases and resolve them later on.
It is ok to suppress even whole inspection rule for some time.

rnveach · 2018-12-02T18:53:01Z

it is reasonable to suppress complicated cases and resolve them later on

I am working through what I can and suppressing the rest.

Duplicated code fragment (77)

I am not finding this in the local intellij to disable. I don't know inspection name.

Deprecated elements (2)

~~I am not sure what to do with this since it is in POM.~~ It can be suppressed.

Main

These are already suppressed but the new version seems to be ignoring the suppression so I am not sure what to do.

rnveach · 2018-12-02T19:49:50Z

Class has no dependencies or dependents in its module

This should be ClassUnconnectedToPackage and it is already disabled, so I don't know why violations are showing.

Class 'TokenTypes' has too many dependents

Again, this is ClassWithTooManyDependents and should already be suppressed.

romani

please do not add more fixes to this PR, lets merge this PR quicker

items to improve:

romani · 2018-12-02T20:03:48Z

config/intellij-idea-inspections.xml

@@ -1136,7 +1136,8 @@
                     enabled_by_default="true"/>
    <inspection_tool class="ClassOnlyUsedInOneModule" enabled="true" level="ERROR"
                     enabled_by_default="true"/>
-    <inspection_tool class="ClassOnlyUsedInOnePackage" enabled="true" level="ERROR"
+    <!-- packages should be where we define them. -->


in library project ...... other projects might use it.

Are you saying you want me to enable this but just use noinspection suppressions?

No, keep disabled but update wording as I proposed

romani · 2018-12-02T20:19:34Z

config/intellij-idea-inspections.xml

-                     enabled_by_default="true"/>
+                     enabled_by_default="true">
+        <!-- is still provides valuable information to programmer -->
+        <option name="REPORT_INACCESSIBLE" value="false" />


I am not agree here a bit.

current violations:

Declaration has problems in Javadoc references AbstractJavadocCheck Symbol 'firstNonTightHtmlTag' is inaccessible from here UniquePropertiesCheckTest Symbol 'getLineNumber(FileText, String)' is inaccessible from here

Symbol 'firstNonTightHtmlTag' is inaccessible from here

it is not reasonable to provide link to private field. Second link in this javadoc is valid and only it should be present.
it gives enough context for user. link to private field is not required.

Symbol 'getLineNumber(FileText, String)' is inaccessible from here

we need to do suppression of this case, usage of reflection in tests is hack and it is a problem of test, javadoc over test should explain all, and it will contain suppression and explanation of suppression. It is good practice to allow hacks but force engineer to provide a lot of documentation/suppressions to show that it is not easy way :) .

romani · 2018-12-02T20:24:10Z

config/intellij-idea-inspections.xml

@@ -3409,6 +3413,9 @@
    <inspection_tool class="OnDemandImport" enabled="true" level="ERROR" enabled_by_default="true"/>
    <inspection_tool class="OneWayWebMethod" enabled="false" level="WARNING"
                     enabled_by_default="false"/>
+    <!-- whole purpose of optional is that it may not always be present -->
+    <inspection_tool class="OptionalGetWithoutIsPresent" enabled="false" level="ERROR"
+                     enabled_by_default="false" />


not good.

Optional.get() is called without isPresent() check PackageObjectFactory 'Optional.get()' without 'isPresent()' check TokenUtilTest 'Optional.get()' without 'isPresent()' check

This is design problem and/or misusage/illusion of Optional class, without such check for presence , there will be exception - https://docs.oracle.com/javase/8/docs/api/java/util/Optional.html#get-- .
Solution is smth like - https://stackoverflow.com/a/38725479

PackageObjectFactory

Code is .get().getKey() so we can't just return null either. Also, this code is wrapped by containsValue of the same list, so I highly doubt an if/else check would pass code coverage.
I think we should suppress this, you agree?

Other class was fixed.

Please keep suppression by separate our issue, we will discuss it separately.

I can confirm code coverage won't be satisfied, but I think I see a way to rewrite the code.

@romani I rewrote the code. We basically rewrote the containsValue twice by different means, so I removed the main one and now we just rely on the one using optional.

romani · 2018-12-02T20:28:40Z

config/intellij-idea-inspections.xml

+    <!-- unstable doesn't mean it isn't safe to use, just might
+         change between versions and isn't stable -->
+    <inspection_tool class="UnstableApiUsage" enabled="false" level="ERROR"
+                     enabled_by_default="false" />


lets update configuration if it and remove guava.Beta from list:

romani · 2018-12-02T20:34:42Z

pom.xml

@@ -835,7 +835,7 @@
                    <exclude>com.puppycrawl.tools.checkstyle.gui.ParseTreeTableModel*</exclude>
                    <exclude>com.puppycrawl.tools.checkstyle.gui.TreeTableCellRenderer*</exclude>
                    <exclude>com.puppycrawl.tools.checkstyle.gui.TreeTableModelAdapter*</exclude>
-                    <!-- Deprecated classes -->
+                    <!-- suppress XmlDeprecatedElement Deprecated classes -->


from inspection description:
This inspection checks for deprecated XML elements. The elements can be marked by XML comment or documentation tag with text "deprecated".

new comment looks very weird. If inspection do not like just a word "deprecated",
lets make comment 

romani · 2018-12-02T20:36:56Z

src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java

@@ -165,6 +165,8 @@ private ConfigurationLoader(final PropertyResolver overrideProps,
    /**
     * Creates mapping between local resources and dtd ids.
     * @return map between local resources and dtd ids.
+     * @noinspection MethodOnlyUsedFromInnerClass method must be static,
+     *     and inner class isn't static


inspections do not support definition of reason of suppress, in tag @noinspection there should be only list of inspection names.

method must be static, and inner class isn't static should be moved to javadoc description. Method is private, it will not be visible in HTML.

Done.

I just made it a new sentence. Let me know if you want it documented another way.

romani · 2018-12-02T20:38:41Z

src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java

@@ -471,7 +473,7 @@ public static Configuration loadConfiguration(InputSource configSource,
     * @throws CheckstyleException if the string contains an opening
     *                           {@code ${} without a closing
     *                           {@code }}
-     * @noinspection MethodWithMultipleReturnPoints
+     * @noinspection MethodWithMultipleReturnPoints, MethodOnlyUsedFromInnerClass


please place reason of suppress to javadoc description, sad that I already missed this for another suppression.

romani · 2018-12-02T20:41:37Z

src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java

@@ -283,6 +283,10 @@ public LocalizedMessage(
                sourceClass, customMessage);
    }

+    /**
+     * Indicates whether some other object is "equal to" this one.
+     * @noinspection EqualsCalledOnEnumConstant


we need to explain suppressions in javadoc, unfortunate side effect.
We always keep brief reason of suppression in code or to keep link to web, with detailed explanation. unfortunately here we will damage a big our javadoc, but is better than keep such explanation in code

romani · 2018-12-02T20:47:59Z

src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressWithPlainTextCommentFilter.java

@@ -290,6 +290,10 @@ protected Suppression(
            }
        }

+        /**
+         * Indicates whether some other object is "equal to" this one.
+         * @noinspection EqualsCalledOnEnumConstant


reason of suppression to javdoc descrition

romani · 2018-12-02T20:48:04Z

src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressionCommentFilter.java

@@ -414,6 +414,10 @@ public int compareTo(Tag object) {
            return result;
        }

+        /**
+         * Indicates whether some other object is "equal to" this one.
+         * @noinspection EqualsCalledOnEnumConstant


reason of suppression to javdoc descrition

romani · 2018-12-02T23:25:06Z

@rnveach, I merged few PRs with suppressions for cases that you are not sure, 2 issues were raised against inspections.
Please fix your cases and we should end up in green CI.
If some leftovers come up, we will fix them in separate PR.

romani · 2018-12-03T02:34:12Z

Please rebase to resolve other issues.

rnveach · 2018-12-03T02:38:57Z

@romani rebased and fixed all issues.

rnveach · 2018-12-03T03:12:42Z

Only TC violations left are Duplicated code fragment and Class independent of its module.

romani · 2018-12-03T04:25:14Z

Duplicated code fragment

Is TC issue, let's wait for support reply.

Class independent of its module

Need to be addressed or suppressed. It is the same as others not working suppressions, we are to disable inspection.

rnveach commented Dec 2, 2018

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java Outdated Show resolved Hide resolved

romani requested changes Dec 2, 2018

View reviewed changes

src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java Outdated Show resolved Hide resolved

rnveach force-pushed the issue_6241 branch from 128cb30 to 2e5289e Compare December 2, 2018 18:51

rnveach force-pushed the issue_6241 branch from 2e5289e to 351cc3f Compare December 2, 2018 19:15

rnveach force-pushed the issue_6241 branch from 5f36e0a to 953d645 Compare December 2, 2018 20:00

romani requested changes Dec 2, 2018

View reviewed changes

rnveach force-pushed the issue_6241 branch from 2645967 to 92dad3f Compare December 3, 2018 02:38

Issue checkstyle#6241: resolved teamcity 2018.3 violations

fbfe6e9

rnveach force-pushed the issue_6241 branch from 92dad3f to fbfe6e9 Compare December 3, 2018 03:26

romani merged commit bc4fe1e into checkstyle:master Dec 3, 2018

rnveach deleted the issue_6241 branch December 3, 2018 04:42

Issue #6241: resolved teamcity 2018.3 violations #6242

Issue #6241: resolved teamcity 2018.3 violations #6242

Conversation

rnveach commented Dec 2, 2018 • edited

romani left a comment

Choose a reason for hiding this comment

romani commented Dec 2, 2018

rnveach commented Dec 2, 2018 • edited

rnveach commented Dec 2, 2018

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Dec 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Dec 2, 2018

romani commented Dec 3, 2018

rnveach commented Dec 3, 2018

rnveach commented Dec 3, 2018

romani commented Dec 3, 2018 • edited

rnveach commented Dec 2, 2018 •

edited

rnveach commented Dec 2, 2018 •

edited

rnveach Dec 3, 2018 •

edited

romani commented Dec 3, 2018 •

edited