Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3.0-M1
    • Component/s: None
    • Labels:
      None

      Description

      Upgrade to Checkstyle 5.1.
      It has quite a few fixes and also support for Java 5. Since Click is using Java 5 now this version would be more useful.

      Checkstyle 5.1 (but 5.0 too) also contains many checks not used by Click - /build/checkstyle-checks.xml is using just a few of them.

        Activity

        Hide
        Adrian A. added a comment -

        I updated to checkstyle 5.1, but:

        > Checkstyle 5.1 (but 5.0 too) also contains many checks not used by
        > Click - /build/checkstyle-checks.xml is using just a few of them.
        what exact checks do you have in mind to make use of, besides the actual used ones?

        There are just too many checks there, that Click is not using, and we can't just use them all (many don't make sense to Click at all) .

        Show
        Adrian A. added a comment - I updated to checkstyle 5.1, but: > Checkstyle 5.1 (but 5.0 too) also contains many checks not used by > Click - /build/checkstyle-checks.xml is using just a few of them. what exact checks do you have in mind to make use of, besides the actual used ones? There are just too many checks there, that Click is not using, and we can't just use them all (many don't make sense to Click at all) .
        Hide
        George Stan added a comment -

        > what exact checks do you have in mind to make use of, besides the actual used ones?
        1. Annotations:
        http://checkstyle.sourceforge.net/config_annotation.html

        6. More from Javadoc:
        http://checkstyle.sourceforge.net/config_javadoc.html

        • not just the JavadocPackage check

        The main point would be that since Click is using Checkstyle, than why not make use of it's entire power (now it looks like it's using only a small percentage of it).

        This might be a separate issue:
        =========================
        I'm not an expert of Checkstyle, but even this would be a fantastic improvement: http://checkstyle.sourceforge.net/extending.html

        • to make some custom extended checks
          1 - Click specific
          2 - Click based webapp specific
          3 - to enforce best practices

        1. These might be for the Click developers (and Click component developers) to have extra checks not covered by Checkstyle, but to enforce conventions: e.g. the first Control constructor parameter is the name of the control, etc.

        2. Click catches allot of problems and it's doing allot of checks at runtime, but they consume quite a few cycles.
        What if those checks would be "configurable"/ optional at runtime (so would consume no resources/cycles at all if turned off), but instead they would be as Checkstyle based tasks, that the user can perform at build time?
        In performance intensive installations this could be a fantastic improvement.

        3. This would allow to enforce some of the best practices described in click docs.

        Show
        George Stan added a comment - > what exact checks do you have in mind to make use of, besides the actual used ones? 1. Annotations: http://checkstyle.sourceforge.net/config_annotation.html Click is using java 5. 2. Empty Block: http://checkstyle.sourceforge.net/config_blocks.html#EmptyBlock 3. Headers: http://checkstyle.sourceforge.net/config_header.html to enforce the Apache header from checkstyle, but also to ensure it's consistency. 4. Coding: http://checkstyle.sourceforge.net/config_coding.html to use quite a few from coding (too many to enumerate them here. 5. Duplicate Code: http://checkstyle.sourceforge.net/config_duplicates.html 6. More from Javadoc: http://checkstyle.sourceforge.net/config_javadoc.html not just the JavadocPackage check The main point would be that since Click is using Checkstyle, than why not make use of it's entire power (now it looks like it's using only a small percentage of it). This might be a separate issue: ========================= I'm not an expert of Checkstyle, but even this would be a fantastic improvement: http://checkstyle.sourceforge.net/extending.html to make some custom extended checks 1 - Click specific 2 - Click based webapp specific 3 - to enforce best practices 1. These might be for the Click developers (and Click component developers) to have extra checks not covered by Checkstyle, but to enforce conventions: e.g. the first Control constructor parameter is the name of the control, etc. 2. Click catches allot of problems and it's doing allot of checks at runtime, but they consume quite a few cycles. What if those checks would be "configurable"/ optional at runtime (so would consume no resources/cycles at all if turned off), but instead they would be as Checkstyle based tasks, that the user can perform at build time? In performance intensive installations this could be a fantastic improvement. 3. This would allow to enforce some of the best practices described in click docs.
        Hide
        Adrian A. added a comment -

        >> what exact checks do you have in mind to make use of, besides the actual used ones?
        >1. Annotations:
        > http://checkstyle.sourceforge.net/config_annotation.html
        Done.

        >2. Empty Block:
        > http://checkstyle.sourceforge.net/config_blocks.html#EmptyBlock
        It's there - line 156, but not active: Click is using many empty blocks right now.

        >3. Headers:
        > http://checkstyle.sourceforge.net/config_header.html
        Done.

        >4. Coding:
        > http://checkstyle.sourceforge.net/config_coding.html
        > - (too many to enumerate them here.
        IMHO also too many to add and try them now . In fact they look so many that it would be an issue on itself to use, configure and tune them.

        > 5. Duplicate Code:
        > http://checkstyle.sourceforge.net/config_duplicates.html
        I would have liked this greatly but it's buggy: it shows the Apache header as duplicate code - so it's not smart at all. If I see right it's simply using a regular expression for this - not something like the IntelliJ inspections.

        >6. More from Javadoc:
        > http://checkstyle.sourceforge.net/config_javadoc.html
        Done.

        7. I added a few more that made sense for me. See a diff of the checkstyle file for this.
        8. There are a few others commented, because they need more test/tune before enforcing them upon the source code.

        > This might be a separate issue .... http://checkstyle.sourceforge.net/extending.html
        Interesting concept (thought myself about it while using checkstyle), but please move it to another issue/wiki/discussion because I consider this issue: "upgrade to checkstyle 5.1" as closed .

        Show
        Adrian A. added a comment - >> what exact checks do you have in mind to make use of, besides the actual used ones? >1. Annotations: > http://checkstyle.sourceforge.net/config_annotation.html Done. >2. Empty Block: > http://checkstyle.sourceforge.net/config_blocks.html#EmptyBlock It's there - line 156, but not active: Click is using many empty blocks right now. >3. Headers: > http://checkstyle.sourceforge.net/config_header.html Done. >4. Coding: > http://checkstyle.sourceforge.net/config_coding.html > - (too many to enumerate them here. IMHO also too many to add and try them now . In fact they look so many that it would be an issue on itself to use, configure and tune them. > 5. Duplicate Code: > http://checkstyle.sourceforge.net/config_duplicates.html I would have liked this greatly but it's buggy: it shows the Apache header as duplicate code - so it's not smart at all. If I see right it's simply using a regular expression for this - not something like the IntelliJ inspections. >6. More from Javadoc: > http://checkstyle.sourceforge.net/config_javadoc.html Done. 7. I added a few more that made sense for me. See a diff of the checkstyle file for this. 8. There are a few others commented, because they need more test/tune before enforcing them upon the source code. > This might be a separate issue .... http://checkstyle.sourceforge.net/extending.html Interesting concept (thought myself about it while using checkstyle), but please move it to another issue/wiki/discussion because I consider this issue: "upgrade to checkstyle 5.1" as closed .

          People

          • Assignee:
            Adrian A.
            Reporter:
            George Stan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development