Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.4
    • Component/s: General
    • Labels:
      None
    • Number of attachments :
      1

      Description

      I would like to tune our checkstyle settngs. One change is to not complain about empty blocks as long as there is SOME text in there. That allows us to have an empty block as long as we put a comment in there about why it's empty.

      I would like to remove module name="DesignForExtension" – its checks are so stringent that it generates more noise than useful signal. It makes it impossible to have a mostly-checkstyle-complaint-free codebase.

      I was hoping we could change the max line length from 90 to 100. It makes a LOT of warnings go away, and forcing line folding at 90 reduces readability (IMO)

      I was hoping we could add the following checks:

      <module name="TodoComment">
      <property name="format" value="FIXME:"/>
      </module>
      <module name="TodoComment">
      <property name="format" value="TEMP:"/>
      </module>
      <module name="ParameterAssignment">
      <property name="severity" value="info"/>
      </module>
      <module name="MultipleVariableDeclarations">
      <property name="severity" value="info"/>
      </module>
      <module name="MultipleStringLiterals">
      <metadata name="com.atlassw.tools.eclipse.checkstyle.lastEnabledSeverity" value="info"/>
      <property name="severity" value="ignore"/>
      </module>
      <module name="ModifiedControlVariable">
      <property name="severity" value="info"/>
      </module>
      <module name="JUnitTestCase"/>
      <module name="FinalLocalVariable">
      <metadata name="com.atlassw.tools.eclipse.checkstyle.lastEnabledSeverity" value="info"/>
      <property name="severity" value="ignore"/>
      <property name="tokens" value="PARAMETER_DEF,VARIABLE_DEF"/>
      </module>
      <module name="CovariantEquals"/>
      <module name="JavadocStyle"/>

      Rather than attach a patch, I'm just going to attach my propsoed src/tools/checkstyle/castor-main-checks.xml

      Thoughts?

        Activity

        Hide
        Edward Kuns added a comment -

        As an unrelated checkstyle comment, I've found that the eclipse checkstyle plugin REQUIRES backslash-style folder names to function under Windows, but of course cannot use such names under Linux. How annoying. That means that someone working under UNIX has to hand-modify the .checkstyle file to point to somewhere meaningful under UNIX.

        Show
        Edward Kuns added a comment - As an unrelated checkstyle comment, I've found that the eclipse checkstyle plugin REQUIRES backslash-style folder names to function under Windows, but of course cannot use such names under Linux. How annoying. That means that someone working under UNIX has to hand-modify the .checkstyle file to point to somewhere meaningful under UNIX.
        Hide
        Ralf Joachim added a comment -

        Following changes sound reasonable to me:

        • allow empty blocks with comments
        • extend line length from 90 to 100 characters
        • allow FIXME and TEMP as todo comments

        I don' tlike to relax:

        • design for extension as most areas of code where this apears should be changed and we would not have a signal for that without the rule

        Can you please explain a bit on the rest of of the rules.

        Show
        Ralf Joachim added a comment - Following changes sound reasonable to me: allow empty blocks with comments extend line length from 90 to 100 characters allow FIXME and TEMP as todo comments I don' tlike to relax: design for extension as most areas of code where this apears should be changed and we would not have a signal for that without the rule Can you please explain a bit on the rest of of the rules.
        Hide
        Edward Kuns added a comment -

        On design for extension – it generates SO MUCH noise!!! Do you believe that most places we see this we should change the code? That means making all public non-empty non-abstract methods final to prevent someone from extending a class, which doesn't seem friendly.

        The other rules:

        • Parameter assignment - goal: prevent people from changing the value of a method parameter
          *MultipleVariableDeclarations - goal: prevent on one line: float a; int i; String name; as those should be on separate lines. This would need a tweak to not give unnecessary complaints on a for() statement
        • MultipleStringLiterals - goal: catch places where the same string literal occurs more than once in a source file – needs to be tweaked as it's currently too sensitive, so I have it listed but off
        • ModifiedControlVariable - goal: prevent one from changing "i" if it is the control variable of a for() loop
        • JUnitTestCase - various correctness checks for any class that is a JUnit test csae
        • FinalLocalVariable - too much noise for now, but if a local variable is assigned once and never changed, note that it can be set to final
        • CovariantEquals - check for equals(String) or anything other than equals(Object) to prevent places where people WANT to override equals() but provdie the wrong method signature
        • JavadocStyle - do checks on the correctness of JavaDoc comments
        Show
        Edward Kuns added a comment - On design for extension – it generates SO MUCH noise!!! Do you believe that most places we see this we should change the code? That means making all public non-empty non-abstract methods final to prevent someone from extending a class, which doesn't seem friendly. The other rules: Parameter assignment - goal: prevent people from changing the value of a method parameter *MultipleVariableDeclarations - goal: prevent on one line: float a; int i; String name; as those should be on separate lines. This would need a tweak to not give unnecessary complaints on a for() statement MultipleStringLiterals - goal: catch places where the same string literal occurs more than once in a source file – needs to be tweaked as it's currently too sensitive, so I have it listed but off ModifiedControlVariable - goal: prevent one from changing "i" if it is the control variable of a for() loop JUnitTestCase - various correctness checks for any class that is a JUnit test csae FinalLocalVariable - too much noise for now, but if a local variable is assigned once and never changed, note that it can be set to final CovariantEquals - check for equals(String) or anything other than equals(Object) to prevent places where people WANT to override equals() but provdie the wrong method signature JavadocStyle - do checks on the correctness of JavaDoc comments
        Hide
        Ralf Joachim added a comment -

        According to yur explanation I am fine with all changes but not with removeal of the design for extension info complaints. We don't need to make all public non-empty non-abstract methods final as the complaints are at info level. The main reason is that they are the only way to force people to think if the method should be not final by intention or if they simply forgot to declare it final. Therefore I suggest to go for the rest of the changes.

        Show
        Ralf Joachim added a comment - According to yur explanation I am fine with all changes but not with removeal of the design for extension info complaints. We don't need to make all public non-empty non-abstract methods final as the complaints are at info level. The main reason is that they are the only way to force people to think if the method should be not final by intention or if they simply forgot to declare it final. Therefore I suggest to go for the rest of the changes.

          People

          • Assignee:
            Edward Kuns
            Reporter:
            Edward Kuns
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: