OpenJPA
  1. OpenJPA
  2. OPENJPA-1015

Enforce 80-column line width for source code

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.0-M2
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      There used to be a rule of 80-column width for source code.
      That rule is slowly yielding.

      Should we add a test case to catch such violation?
      Or should we not reignite the age-old battle about line width, placement of brackets etc?

      1. TestLineWidth.java
        5 kB
        Pinaki Poddar
      2. line80.txt
        17 kB
        Pinaki Poddar
      3. OPENJPA-1015-max80chars.patch
        262 kB
        B.J. Reed
      4. OPENJPA-1015-newlineendoffile.patch
        13 kB
        B.J. Reed
      5. OPENJPA-1015-checkstyle.xml.patch
        2 kB
        Donald Woods
      6. OPENJPA-1015-audit-output.patch
        0.5 kB
        Donald Woods
      7. OPENJPA-1015-plugin-version.patch
        0.6 kB
        Donald Woods
      8. OPENJPA-1015-test.patch
        585 kB
        B.J. Reed

        Issue Links

          Activity

          Hide
          Michael Dick added a comment -

          >> Anyone opposed to doing the tests as well?

          >I am, at least temporarily.
          >1. I am checking in auto-generated source code for meta-model classes in the test directories. The program that >generates these source code do not understand char-width limit. Making them smart in that respect is not >currently in my priority list. Till that is done any policy in this regard should not block commit.

          Fixed now

          >2. While everyone agrees on a limit, there is no agreement on 80 char per se.

          Please reply to dev@openjpa where this discussion was raised - this JIRA issue is resolved and isn't the best place to carry on a discussion.

          >3. Test code had been kept out of width limit traditionally. This looks like a new policy. Should be part of a >separate discussion than this one whose original purpose was to recognize an existing policy.

          Interesting, can you show me where we discussed excluding test code from the conventions? I don't remember seeing that anywhere.

          >4. I see violation of other rules in source tree that are not checked now. If compliance is the main goal that >focus should be increasing the variety of compliance rules on the source tree rather than including Test
          >sources.

          Go for it. But the rules should apply to all the source, not just main source. Test code needs to be readable too. It provides a good starting point for many users.

          Show
          Michael Dick added a comment - >> Anyone opposed to doing the tests as well? >I am, at least temporarily. >1. I am checking in auto-generated source code for meta-model classes in the test directories. The program that >generates these source code do not understand char-width limit. Making them smart in that respect is not >currently in my priority list. Till that is done any policy in this regard should not block commit. Fixed now >2. While everyone agrees on a limit, there is no agreement on 80 char per se. Please reply to dev@openjpa where this discussion was raised - this JIRA issue is resolved and isn't the best place to carry on a discussion. >3. Test code had been kept out of width limit traditionally. This looks like a new policy. Should be part of a >separate discussion than this one whose original purpose was to recognize an existing policy. Interesting, can you show me where we discussed excluding test code from the conventions? I don't remember seeing that anywhere. >4. I see violation of other rules in source tree that are not checked now. If compliance is the main goal that >focus should be increasing the variety of compliance rules on the source tree rather than including Test >sources. Go for it. But the rules should apply to all the source, not just main source. Test code needs to be readable too. It provides a good starting point for many users.
          Hide
          Pinaki Poddar added a comment -

          > Anyone opposed to doing the tests as well?

          I am, at least temporarily.
          1. I am checking in auto-generated source code for meta-model classes in the test directories. The program that generates these source code do not understand char-width limit. Making them smart in that respect is not currently in my priority list. Till that is done any policy in this regard should not block commit.

          2. While everyone agrees on a limit, there is no agreement on 80 char per se.

          3. Test code had been kept out of width limit traditionally. This looks like a new policy. Should be part of a separate discussion than this one whose original purpose was to recognize an existing policy.

          4. I see violation of other rules in source tree that are not checked now. If compliance is the main goal that focus should be increasing the variety of compliance rules on the source tree rather than including Test sources.

          Show
          Pinaki Poddar added a comment - > Anyone opposed to doing the tests as well? I am, at least temporarily. 1. I am checking in auto-generated source code for meta-model classes in the test directories. The program that generates these source code do not understand char-width limit. Making them smart in that respect is not currently in my priority list. Till that is done any policy in this regard should not block commit. 2. While everyone agrees on a limit, there is no agreement on 80 char per se. 3. Test code had been kept out of width limit traditionally. This looks like a new policy. Should be part of a separate discussion than this one whose original purpose was to recognize an existing policy. 4. I see violation of other rules in source tree that are not checked now. If compliance is the main goal that focus should be increasing the variety of compliance rules on the source tree rather than including Test sources.
          Hide
          Michael Dick added a comment -

          Resolved in trunk. Could be moved to 1.3.x too though.

          Show
          Michael Dick added a comment - Resolved in trunk. Could be moved to 1.3.x too though.
          Hide
          B.J. Reed added a comment -

          Attaching very large OPENJPA-1015-test patch. patch fixes all line length issues for the test cases. pom.xml has been updated to include the check of the test path during the mvn install and the mvn site builds.

          Show
          B.J. Reed added a comment - Attaching very large OPENJPA-1015 -test patch. patch fixes all line length issues for the test cases. pom.xml has been updated to include the check of the test path during the mvn install and the mvn site builds.
          Hide
          Michael Dick added a comment -

          I've created a subtask to enable the newline rule and moved B.J.'s patch there in order to confine the svn changes associated with this issue to a manageable number (especially when moved to branches).

          The remaining work to be done for this issue is to apply the changes to testcases as well and then determine which branches should also include this change.

          Show
          Michael Dick added a comment - I've created a subtask to enable the newline rule and moved B.J.'s patch there in order to confine the svn changes associated with this issue to a manageable number (especially when moved to branches). The remaining work to be done for this issue is to apply the changes to testcases as well and then determine which branches should also include this change.
          Hide
          Michael Dick added a comment -

          Sorry didn't notice Albert's update until now. This is an old issue with "child" projects in maven and static resources. I'll move checkstyle to openjpa-project and update the path to be a relative one.

          Thanks for catching it Albert.

          Show
          Michael Dick added a comment - Sorry didn't notice Albert's update until now. This is an old issue with "child" projects in maven and static resources. I'll move checkstyle to openjpa-project and update the path to be a relative one. Thanks for catching it Albert.
          Hide
          Albert Lee added a comment -

          It looks like the checkstyle.xml needs to go into each module's root (i.e. openjpa-persistence-jdbc\checkstyle.xml) so that build can be performed.

          E.g.

          cd openjpa-persistence-jdbc
          mvn test .......

          Albert Lee.

          Show
          Albert Lee added a comment - It looks like the checkstyle.xml needs to go into each module's root (i.e. openjpa-persistence-jdbc\checkstyle.xml) so that build can be performed. E.g. cd openjpa-persistence-jdbc mvn test ....... Albert Lee.
          Hide
          Donald Woods added a comment -

          Mike, agree that we should also check the test sources.

          Show
          Donald Woods added a comment - Mike, agree that we should also check the test sources.
          Hide
          Donald Woods added a comment -

          Add the maven-checkstyle-plugin with the current version of 2.2 in the pluginManagement section to control which version is used for the builds.

          Show
          Donald Woods added a comment - Add the maven-checkstyle-plugin with the current version of 2.2 in the pluginManagement section to control which version is used for the builds.
          Hide
          Michael Dick added a comment -

          Thanks for the patch Don, that's much more helpful.

          Note that we're not checking test files at the moment. The change to add that in would be to add
          <includeTestSourceDirectory>true</includeTestSourceDirectory>
          to the <configuration> tag.

          I'm expecting fixing those to be another big change. Anyone opposed to doing the tests as well?

          Show
          Michael Dick added a comment - Thanks for the patch Don, that's much more helpful. Note that we're not checking test files at the moment. The change to add that in would be to add <includeTestSourceDirectory>true</includeTestSourceDirectory> to the <configuration> tag. I'm expecting fixing those to be another big change. Anyone opposed to doing the tests as well?
          Hide
          Donald Woods added a comment -

          Patch which turns on auditing output, so we know which file failed the check.
          Causes output listing the failing file(s), like -

          [INFO] [checkstyle:checkstyle

          {execution: default}

          ]
          [INFO] Starting audit...
          /Users/drwoods/openjpa/trunk/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/DB2Dictionary.java:848: Line is longer than 80 characters.
          Audit done.

          [INFO] ------------------------------------------------------------------------
          [ERROR] BUILD ERROR
          [INFO] ------------------------------------------------------------------------
          [INFO] An error has occurred in Checkstyle report generation.

          Embedded error: There are 1 checkstyle errors.

          Show
          Donald Woods added a comment - Patch which turns on auditing output, so we know which file failed the check. Causes output listing the failing file(s), like - [INFO] [checkstyle:checkstyle {execution: default} ] [INFO] Starting audit... /Users/drwoods/openjpa/trunk/openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/DB2Dictionary.java:848: Line is longer than 80 characters. Audit done. [INFO] ------------------------------------------------------------------------ [ERROR] BUILD ERROR [INFO] ------------------------------------------------------------------------ [INFO] An error has occurred in Checkstyle report generation. Embedded error: There are 1 checkstyle errors.
          Hide
          Michael Dick added a comment -

          Sorry about that, obviously worked for me last night since I had the file.

          Show
          Michael Dick added a comment - Sorry about that, obviously worked for me last night since I had the file.
          Hide
          Donald Woods added a comment -

          Albert, looks like a svn add was missed when the patch was applied. Here is a patch <OPENJPA-1015-checkstyle.xml.patch> that adds just the missing checkstyle.xml file under trunk/

          Show
          Donald Woods added a comment - Albert, looks like a svn add was missed when the patch was applied. Here is a patch < OPENJPA-1015 -checkstyle.xml.patch> that adds just the missing checkstyle.xml file under trunk/
          Hide
          Albert Lee added a comment -

          After applying/updating to the latest commits, I am getting the folllowing error when "mvn clean install -DskipTests=true":

          [INFO] ------------------------------------------------------------------------
          [INFO] Building OpenJPA Utilities
          [INFO] task-segment: [clean, install]
          [INFO] ------------------------------------------------------------------------
          [INFO] [clean:clean]
          [INFO] Deleting file set: C:\a.tools.alleetp\wasx.serv1\openjpa.cur\openjpa-lib\target (included: [**], excluded: [])
          [INFO] [checkstyle:checkstyle

          {execution: default}

          ]
          [INFO] ------------------------------------------------------------------------
          [ERROR] BUILD ERROR
          [INFO] ------------------------------------------------------------------------
          [INFO] An error has occurred in Checkstyle report generation.

          Embedded error: Unable to find configuration file at location checkstyle.xml
          Could not find resource 'checkstyle.xml'.
          [INFO] ------------------------------------------------------------------------
          [INFO] For more information, run Maven with the -e switch
          [INFO] ------------------------------------------------------------------------
          [INFO] Total time: 6 seconds

          Where Is this checkstyle.xml file and is it supposedly part of the commit or I have to create it somehow?

          Thanks,

          Show
          Albert Lee added a comment - After applying/updating to the latest commits, I am getting the folllowing error when "mvn clean install -DskipTests=true": [INFO] ------------------------------------------------------------------------ [INFO] Building OpenJPA Utilities [INFO] task-segment: [clean, install] [INFO] ------------------------------------------------------------------------ [INFO] [clean:clean] [INFO] Deleting file set: C:\a.tools.alleetp\wasx.serv1\openjpa.cur\openjpa-lib\target (included: [**] , excluded: []) [INFO] [checkstyle:checkstyle {execution: default} ] [INFO] ------------------------------------------------------------------------ [ERROR] BUILD ERROR [INFO] ------------------------------------------------------------------------ [INFO] An error has occurred in Checkstyle report generation. Embedded error: Unable to find configuration file at location checkstyle.xml Could not find resource 'checkstyle.xml'. [INFO] ------------------------------------------------------------------------ [INFO] For more information, run Maven with the -e switch [INFO] ------------------------------------------------------------------------ [INFO] Total time: 6 seconds Where Is this checkstyle.xml file and is it supposedly part of the commit or I have to create it somehow? Thanks,
          Hide
          Michael Dick added a comment -

          I've committed the 80 character width patch for trunk with a few changes. The main change is to make the check part of a normal build - not just a report that you can optionally generate.

          If you do a normal maven build and you have a file that violates the rule you'll get an error message like this one :

          [INFO] ------------------------------------------------------------------------
          [INFO] Building OpenJPA Utilities
          [INFO] task-segment: [compile]
          [INFO] ------------------------------------------------------------------------
          [INFO] [checkstyle:checkstyle

          {execution: default}

          ]
          [INFO] ------------------------------------------------------------------------
          [ERROR] BUILD ERROR
          [INFO] ------------------------------------------------------------------------
          [INFO] An error has occurred in Checkstyle report generation.

          Embedded error: There are 1 checkstyle errors.
          [INFO] ------------------------------------------------------------------------
          [INFO] For more information, run Maven with the -e switch
          [INFO] ------------------------------------------------------------------------

          Further experimentation may show a way to print the offending file. As it is today one would have to run svn diff and manually inspect the files that have been changed. For smaller commits this is easily done, larger commits may want to run mvn site and examine the full checkstyle report to see which files exhibit the problem.

          Show
          Michael Dick added a comment - I've committed the 80 character width patch for trunk with a few changes. The main change is to make the check part of a normal build - not just a report that you can optionally generate. If you do a normal maven build and you have a file that violates the rule you'll get an error message like this one : [INFO] ------------------------------------------------------------------------ [INFO] Building OpenJPA Utilities [INFO] task-segment: [compile] [INFO] ------------------------------------------------------------------------ [INFO] [checkstyle:checkstyle {execution: default} ] [INFO] ------------------------------------------------------------------------ [ERROR] BUILD ERROR [INFO] ------------------------------------------------------------------------ [INFO] An error has occurred in Checkstyle report generation. Embedded error: There are 1 checkstyle errors. [INFO] ------------------------------------------------------------------------ [INFO] For more information, run Maven with the -e switch [INFO] ------------------------------------------------------------------------ Further experimentation may show a way to print the offending file. As it is today one would have to run svn diff and manually inspect the files that have been changed. For smaller commits this is easily done, larger commits may want to run mvn site and examine the full checkstyle report to see which files exhibit the problem.
          Hide
          B.J. Reed added a comment -

          The OPENJPA-1015-newlineendoffile.patch file is to fix the 20 or so files that do not end the file with a new line character. It also includes a license in the checkstyle.xml so that will be taken care of.

          This should be applied after the max80character patch, but the 2 .xml files won't go in quite right so the committer will need to do a little work on them.

          Show
          B.J. Reed added a comment - The OPENJPA-1015 -newlineendoffile.patch file is to fix the 20 or so files that do not end the file with a new line character. It also includes a license in the checkstyle.xml so that will be taken care of. This should be applied after the max80character patch, but the 2 .xml files won't go in quite right so the committer will need to do a little work on them.
          Hide
          Michael Dick added a comment -

          Thanks for the patch B.J. A couple comments though :

          1.) Can we put an Apache license at the top of checkstyle.xml? If not we'll have to exclude it from the rat checking (look for the rat plugin in the release profile), and add the appropriate documentation in NOTICE.txt (I think).

          2.) Looks like you fixed a few tabs along the way. It isn't a big deal once I checked for them.

          Show
          Michael Dick added a comment - Thanks for the patch B.J. A couple comments though : 1.) Can we put an Apache license at the top of checkstyle.xml? If not we'll have to exclude it from the rat checking (look for the rat plugin in the release profile), and add the appropriate documentation in NOTICE.txt (I think). 2.) Looks like you fixed a few tabs along the way. It isn't a big deal once I checked for them.
          Hide
          B.J. Reed added a comment -

          Attaching OPENJPA-1015-max80chars.patch. This is the first of many large patches for fixing code style issues. This adds the maven-checkstyle-plugin to the report part of the build and a temporary checkstyle.xml to the build. The idea is that as we get closer to the Sun codestyle recommendations, we can just remove this file and update the pom.xml to use the defaults. After using mvn site, you can check the target/site/checkstyle file to see how many style issues were found.

          85% of the line overruns found by this plugin were caused by tabs or by extra white space at the end of the line or in comments. Tabs show up as 8 characters on some editors so it is counted as 8 characters by the checkstyle plugin. Tabs seem to be absolutely everywhere, so it would be a good idea for everyone (myself included) to make sure that you are not creating more tabs in files as you check them in.

          Show
          B.J. Reed added a comment - Attaching OPENJPA-1015 -max80chars.patch. This is the first of many large patches for fixing code style issues. This adds the maven-checkstyle-plugin to the report part of the build and a temporary checkstyle.xml to the build. The idea is that as we get closer to the Sun codestyle recommendations, we can just remove this file and update the pom.xml to use the defaults. After using mvn site, you can check the target/site/checkstyle file to see how many style issues were found. 85% of the line overruns found by this plugin were caused by tabs or by extra white space at the end of the line or in comments. Tabs show up as 8 characters on some editors so it is counted as 8 characters by the checkstyle plugin. Tabs seem to be absolutely everywhere, so it would be a good idea for everyone (myself included) to make sure that you are not creating more tabs in files as you check them in.
          Hide
          Craig L Russell added a comment -

          I'm ok with the approach of fixing the 80 column lines and then adding other rules as contributors find time to fix them. Part of the usability and maintainability of our code base is consistency in formatting.

          Show
          Craig L Russell added a comment - I'm ok with the approach of fixing the 80 column lines and then adding other rules as contributors find time to fix them. Part of the usability and maintainability of our code base is consistency in formatting.
          Hide
          Jeremy Bauer added a comment -

          Thanks for checking. Agreed. It would be much more effective as part of the main build. I was concerned that it may significantly increase build times. That doesn't appear to be an issue.

          Show
          Jeremy Bauer added a comment - Thanks for checking. Agreed. It would be much more effective as part of the main build. I was concerned that it may significantly increase build times. That doesn't appear to be an issue.
          Hide
          Michael Dick added a comment -

          I think it's useless if it doesn't execute as part of the main build and prevent successful builds if there are errors (this is why we should only enable a subset of rules).

          If it's only in a profile then it'll get ignored and we'll have to do periodic cleanup commits.

          Running the default report (all sun conventions checked) on trunk took 55 seconds on my laptop (including time to download the plugin and its dependencies). Running a subset of the rules should be quicker (might not be noticeable).

          FWIW my laptop is a Lenovo T60 (1.8x GHz dual core). YMMV.

          Show
          Michael Dick added a comment - I think it's useless if it doesn't execute as part of the main build and prevent successful builds if there are errors (this is why we should only enable a subset of rules). If it's only in a profile then it'll get ignored and we'll have to do periodic cleanup commits. Running the default report (all sun conventions checked) on trunk took 55 seconds on my laptop (including time to download the plugin and its dependencies). Running a subset of the rules should be quicker (might not be noticeable). FWIW my laptop is a Lenovo T60 (1.8x GHz dual core). YMMV.
          Hide
          Jeremy Bauer added a comment -

          Will this plugin execute as part of our main build or as a separate profile/phase/<insert correct maven lingo>? If it is part of our main build, any idea how much time it will add to the build?

          Show
          Jeremy Bauer added a comment - Will this plugin execute as part of our main build or as a separate profile/phase/<insert correct maven lingo>? If it is part of our main build, any idea how much time it will add to the build?
          Hide
          Milosz Tylenda added a comment -

          +1.

          There is a related issue OPENJPA-832 which talks about code formatter for Eclipse.

          Show
          Milosz Tylenda added a comment - +1. There is a related issue OPENJPA-832 which talks about code formatter for Eclipse.
          Hide
          Pinaki Poddar added a comment -

          Let us use the plugin. And begin with 80-char line width rule only and add more rules as we go.

          Show
          Pinaki Poddar added a comment - Let us use the plugin. And begin with 80-char line width rule only and add more rules as we go.
          Hide
          B.J. Reed added a comment -

          I don't mind taking a crack at this using the maven-checkstyle-plugin. If no one objects, I can start creating some patches for each style issue that it finds.

          Show
          B.J. Reed added a comment - I don't mind taking a crack at this using the maven-checkstyle-plugin. If no one objects, I can start creating some patches for each style issue that it finds.
          Hide
          Michael Dick added a comment -

          Part of our original mandate was to use the Sun Java code conventions. Enforcement of the rules has been part of the code review process and we've grown lax on things like brackets for one line if statements.

          Rather than writing a new tool to enforce this rule I'd prefer to use the maven-checkstyle-plugin [1] which validates multiple code formatting rules on each build. If we enable all the Sun code formatting rules today we have thousands of issues to fix which is too unwieldy to address.

          I propose starting with a custom set of rules [2] that only enforces column length. Cleaning up the files in one big commit and then slowly extend the rules (cleaning as we go).

          [1] http://maven.apache.org/plugins/maven-checkstyle-plugin/index.html
          [2] http://maven.apache.org/plugins/maven-checkstyle-plugin/examples/custom-checker-config.html

          I'm happy to add the plugin and start on the line limits if other devs like this approach.

          Show
          Michael Dick added a comment - Part of our original mandate was to use the Sun Java code conventions. Enforcement of the rules has been part of the code review process and we've grown lax on things like brackets for one line if statements. Rather than writing a new tool to enforce this rule I'd prefer to use the maven-checkstyle-plugin [1] which validates multiple code formatting rules on each build. If we enable all the Sun code formatting rules today we have thousands of issues to fix which is too unwieldy to address. I propose starting with a custom set of rules [2] that only enforces column length. Cleaning up the files in one big commit and then slowly extend the rules (cleaning as we go). [1] http://maven.apache.org/plugins/maven-checkstyle-plugin/index.html [2] http://maven.apache.org/plugins/maven-checkstyle-plugin/examples/custom-checker-config.html I'm happy to add the plugin and start on the line limits if other devs like this approach.
          Hide
          Pinaki Poddar added a comment -

          1. A simple test to detect all 80+ characters line in openjpa source code. Harcoded some module paths.

          2. Output showing the list of violated lines. There are 138 affected files each carrying 1-14 lines that exceed 80-char limit.

          First few lines of the output....

          Following 138 files contain lines wider than 80 characters
          1 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationBuilder.java:[179]
          7 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationPersistenceMetaDataParser.java:[31, 32, 538, 590, 595, 599, 603]
          3 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationPersistenceMetaDataSerializer.java:[492, 1240, 1308]
          1 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationPersistenceXMLMetaDataParser.java:[283]

          Show
          Pinaki Poddar added a comment - 1. A simple test to detect all 80+ characters line in openjpa source code. Harcoded some module paths. 2. Output showing the list of violated lines. There are 138 affected files each carrying 1-14 lines that exceed 80-char limit. First few lines of the output.... Following 138 files contain lines wider than 80 characters 1 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationBuilder.java: [179] 7 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationPersistenceMetaDataParser.java: [31, 32, 538, 590, 595, 599, 603] 3 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationPersistenceMetaDataSerializer.java: [492, 1240, 1308] 1 C:\work\openjpa-1.3.0\openjpa-persistence\src\main\java\org\apache\openjpa\persistence\AnnotationPersistenceXMLMetaDataParser.java: [283]

            People

            • Assignee:
              B.J. Reed
              Reporter:
              Pinaki Poddar
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development