Hadoop Common
  1. Hadoop Common
  2. HADOOP-1051

Add checkstyle target to ant build file

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.11.2
    • Fix Version/s: 0.12.0
    • Component/s: build, test
    • Labels:
      None

      Description

      As discussed in HADOOP-948, add a target to allow people to run style checks on the codebase.

      1. checkstyle-v2.patch
        14 kB
        Tom White
      2. checkstyle-errors.html
        829 kB
        Tom White
      3. checkstyle.patch
        14 kB
        Tom White

        Issue Links

          Activity

          Hide
          Doug Cutting added a comment -

          These rules look fine to me, so I committed this. Thanks, Tom!

          (I also made it so that subversion will ignore files named 'checkstyle*' in the lib directory, so that we won't accidentally check this LGPL'd jar in.)

          Show
          Doug Cutting added a comment - These rules look fine to me, so I committed this. Thanks, Tom! (I also made it so that subversion will ignore files named 'checkstyle*' in the lib directory, so that we won't accidentally check this LGPL'd jar in.)
          Hide
          Doug Cutting added a comment -

          > Is it worth excluding attached files that don't have an ASF license granted too?

          Or, if they're patches, adding -1 for that reason?

          Also, rather than checking for other file types, couldn't we instead check that something's indeed a patch file, and ignore all non-patch attachments? The unix 'file' command does a good job of identifying patches as 'RCS/CVS diff output text'.

          Show
          Doug Cutting added a comment - > Is it worth excluding attached files that don't have an ASF license granted too? Or, if they're patches, adding -1 for that reason? Also, rather than checking for other file types, couldn't we instead check that something's indeed a patch file, and ignore all non-patch attachments? The unix 'file' command does a good job of identifying patches as 'RCS/CVS diff output text'.
          Hide
          Tom White added a comment -

          > I'll fix the patch process to omit *.html and *.htm attachments when
          > choosing a patch file.

          Is it worth excluding attached files that don't have an ASF license granted too?

          Show
          Tom White added a comment - > I'll fix the patch process to omit *.html and *.htm attachments when > choosing a patch file. Is it worth excluding attached files that don't have an ASF license granted too?
          Hide
          Tom White added a comment -

          I too don't want to waste time arguing over which set of checks to enforce. Instead I would suggest a lowest common denominator set of checks which are broadly uncontentious, and which committers can run against patches at their own discretion to check that they are basically OK.

          This second patch remove's Doug's spurious warnings and adds David's change for indentation checking.

          I take the point about reasonable people differing on these issues, which is why I would not integrate this into the build tool and instead give the committers the final say.

          Show
          Tom White added a comment - I too don't want to waste time arguing over which set of checks to enforce. Instead I would suggest a lowest common denominator set of checks which are broadly uncontentious, and which committers can run against patches at their own discretion to check that they are basically OK. This second patch remove's Doug's spurious warnings and adds David's change for indentation checking. I take the point about reasonable people differing on these issues, which is why I would not integrate this into the build tool and instead give the committers the final say.
          Hide
          David Bowen added a comment -

          Here's the config change to add indentation checking:

          <module name="Indentation">
          <property name="basicOffset" value="2" />
          <property name="caseIndent" value="2" />
          </module>

          (The default sun_checks.xml does not check indentation.)

          Show
          David Bowen added a comment - Here's the config change to add indentation checking: <module name="Indentation"> <property name="basicOffset" value="2" /> <property name="caseIndent" value="2" /> </module> (The default sun_checks.xml does not check indentation.)
          Hide
          Doug Cutting added a comment -

          > Can the checkstyle jar be checked into the lib directory so that people don't have to download it?

          No. It's available under the LGPL, and Apache policy prohibts redistribution of LGPL'd stuff.

          http://people.apache.org/~cliffs/3party.html

          Show
          Doug Cutting added a comment - > Can the checkstyle jar be checked into the lib directory so that people don't have to download it? No. It's available under the LGPL, and Apache policy prohibts redistribution of LGPL'd stuff. http://people.apache.org/~cliffs/3party.html
          Hide
          David Bowen added a comment -

          +1. I like the idea of doing this. Following code conventions makes code easier to read and to modify.

          It would be good to get the indentation consistent. I don't like the 2-space convention (I much prefer 4-space) but I'd rather have 2-space everywhere than the current situation where the spacing is inconsistent.

          Can the checkstyle jar be checked into the lib directory so that people don't have to download it?

          Show
          David Bowen added a comment - +1. I like the idea of doing this. Following code conventions makes code easier to read and to modify. It would be good to get the indentation consistent. I don't like the 2-space convention (I much prefer 4-space) but I'd rather have 2-space everywhere than the current situation where the spacing is inconsistent. Can the checkstyle jar be checked into the lib directory so that people don't have to download it?
          Hide
          Doug Cutting added a comment -

          +0

          It might be useful to add this. A patch should ideally not increase the number of style warnings. But, first, we'll need to agree on the default settings, which will be contentious. For example, I find the following warnings spurious:

          'cast' is not followed by whitespace.
          '+' should be on a new line.
          '||' should be on a new line.
          '1024' is a magic number.
          '0xffff' is a magic number.
          Using the '.' form of import should be avoided - org.apache.commons.logging..

          I'm even okay with if's that don't have braces when there's no 'else' clause. Indentation tells the story there quite well and it's not a source of errors or misunderstandings. But I suspect there are those who will argue with that.

          On the other hand, I'm bothered by lines over 80 columns and non-2-space indentation (the testing of which you've disabled) while many others are apparently not. However I've managed to collaborate on projects with such people for many years, without any serious problems.

          Reasonable people differ about these things. I fear we could waste a lot of time bickering about the standard style definition at the expense of getting things done. Selective enforcement by committers, with all its pitfalls, may be a more pragmatic route.

          Show
          Doug Cutting added a comment - +0 It might be useful to add this. A patch should ideally not increase the number of style warnings. But, first, we'll need to agree on the default settings, which will be contentious. For example, I find the following warnings spurious: 'cast' is not followed by whitespace. '+' should be on a new line. '||' should be on a new line. '1024' is a magic number. '0xffff' is a magic number. Using the '. ' form of import should be avoided - org.apache.commons.logging. . I'm even okay with if's that don't have braces when there's no 'else' clause. Indentation tells the story there quite well and it's not a source of errors or misunderstandings. But I suspect there are those who will argue with that. On the other hand, I'm bothered by lines over 80 columns and non-2-space indentation (the testing of which you've disabled) while many others are apparently not. However I've managed to collaborate on projects with such people for many years, without any serious problems. Reasonable people differ about these things. I fear we could waste a lot of time bickering about the standard style definition at the expense of getting things done. Selective enforcement by committers, with all its pitfalls, may be a more pragmatic route.
          Hide
          Hadoop QA added a comment -

          -1, because the patch command could not apply the latest attachment http://issues.apache.org/jira/secure/attachment/12352266/checkstyle-errors.html as a patch to trunk revision http://svn.apache.org/repos/asf/lucene/hadoop/trunk/512944. Please note that this message is automatically generated and may represent a problem with the automation system and not the patch. Results are at http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch

          Show
          Hadoop QA added a comment - -1, because the patch command could not apply the latest attachment http://issues.apache.org/jira/secure/attachment/12352266/checkstyle-errors.html as a patch to trunk revision http://svn.apache.org/repos/asf/lucene/hadoop/trunk/512944 . Please note that this message is automatically generated and may represent a problem with the automation system and not the patch. Results are at http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Patch
          Hide
          Tom White added a comment -

          I forgot to mention that you need to download checkstyle 4.3 and install the "all" jar in the lib directory before using the new target.

          Show
          Tom White added a comment - I forgot to mention that you need to download checkstyle 4.3 and install the "all" jar in the lib directory before using the new target.
          Hide
          Tom White added a comment -

          Here is a sample report for the codebase produced by the checkstyle target.

          Show
          Tom White added a comment - Here is a sample report for the codebase produced by the checkstyle target.
          Hide
          Tom White added a comment -

          This patch adds a "checkstyle" target that is independent of other targets. We might want to add it to the "test" target in the future, but for now the idea is that contributors can run it to see if their changes introduce style errors. Similarly, committers can run it to see if patches increase the number of warnings.

          This is a candidate for adding to the Hudson build too.

          The current set of styles that is checked is a cutdown copy of the Sun coding conventions. (It is cutdown since there were a number of checks to do with whitespace that had hundreds of violations, so I removed them for the moment.) We can add or remove checks over time.

          Show
          Tom White added a comment - This patch adds a "checkstyle" target that is independent of other targets. We might want to add it to the "test" target in the future, but for now the idea is that contributors can run it to see if their changes introduce style errors. Similarly, committers can run it to see if patches increase the number of warnings. This is a candidate for adding to the Hudson build too. The current set of styles that is checked is a cutdown copy of the Sun coding conventions. (It is cutdown since there were a number of checks to do with whitespace that had hundreds of violations, so I removed them for the moment.) We can add or remove checks over time.

            People

            • Assignee:
              Tom White
              Reporter:
              Tom White
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development