Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-306

Standardize code style for "import package.*;"

        Details

        • Type: Improvement
        • Status: Closed
        • Priority: Trivial
        • Resolution: Fixed
        • Affects Version/s: None
        • Fix Version/s: 1.0.0-incubating
        • Component/s: None
        • Labels:
          None

          Description

          Our house style does not specify whether/when imports are to be converted to stars. I propose that imports should be converted to stars if there are more than 3 from the same package. Thus:

          import a.b.C1;
import a.b.C2;
import a.b.C3;

          becomes

          import a.b.*;

          when a.b.C4 is added. This is consistent with IntelliJ's default rule.

          It is OK to use stars if there are 3 or fewer uses. Thus removing the use of a.b.C2 would not require imports to be changed.

          Checkstyle has a rule to ban star imports (excluding certain packages) but does not allow them to be limited to a particular number.

            Issue Links

            is related to

            Bug - A problem which impairs or prevents the functions of the product. CALCITE-296 Re-organize package structure

            • Major - Major loss of function.
            • Closed

              Activity

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              vladimirsitnikov Vladimir Sitnikov added a comment -

              Google java style forbids wildcards imports.
              I find it useful:

              • merge conflicts should be less harmful
              • good tools support: easy to configure IDE and checkstyle to forbid wildcards
              • it does not disturbs when coding: IDE collapses imports
              • clear understanding of "which class is used" when looking at bare source code (i.e. github)
              Show
              vladimirsitnikov Vladimir Sitnikov added a comment - Google java style forbids wildcards imports. I find it useful: merge conflicts should be less harmful good tools support: easy to configure IDE and checkstyle to forbid wildcards it does not disturbs when coding: IDE collapses imports clear understanding of "which class is used" when looking at bare source code (i.e. github)
              Hide
              Permalink
              julianhyde Julian Hyde added a comment -

              I could live with that. We should apply this standard when we re-organize packages. It should be sufficient to add a checkstyle AvoidStarImport directive http://checkstyle.sourceforge.net/config_imports.html.

              I also propose that we use the order in the google style guide:
              1. All static imports in a single group
              2. org.apache.optiq imports (if this source file is in the org.apache.optiq package space)
              3. Third-party imports, one group per top-level package, in ASCII sort order
              for example: com, junit, org, sun
              4. java imports
              5. javax imports

              Show
              julianhyde Julian Hyde added a comment - I could live with that. We should apply this standard when we re-organize packages. It should be sufficient to add a checkstyle AvoidStarImport directive http://checkstyle.sourceforge.net/config_imports.html . I also propose that we use the order in the google style guide: 1. All static imports in a single group 2. org.apache.optiq imports (if this source file is in the org.apache.optiq package space) 3. Third-party imports, one group per top-level package, in ASCII sort order for example: com, junit, org, sun 4. java imports 5. javax imports
              Hide
              Permalink
              julianhyde Julian Hyde added a comment -

              we should re-organize imports at the same time that we re-org package structure

              Show
              julianhyde Julian Hyde added a comment - we should re-organize imports at the same time that we re-org package structure
              Hide
              Permalink
              vladimirsitnikov Vladimir Sitnikov added a comment -

              I've tried improving checkstyle's reporting of "invalid import order", and got rejected by checkstyle maintainers: discussion int the dev list, github pull request.

              Thus I would drop any "smart" rules like "if this source file is in the org.apache.optiq package space" since I see very little help from them, and they will definitely catch and annoy optiq contributors.

              I am not sure if we want forking checkstyle. I would spend some time on optiq rather than checkstyle part of it.

              #1, #3, #4 and #5 are fine.

              Show
              vladimirsitnikov Vladimir Sitnikov added a comment - I've tried improving checkstyle's reporting of "invalid import order", and got rejected by checkstyle maintainers: discussion int the dev list , github pull request . Thus I would drop any "smart" rules like "if this source file is in the org.apache.optiq package space" since I see very little help from them, and they will definitely catch and annoy optiq contributors. I am not sure if we want forking checkstyle. I would spend some time on optiq rather than checkstyle part of it. #1, #3, #4 and #5 are fine.
              Hide
              Permalink
              julianhyde Julian Hyde added a comment -

              Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/a0ba73cd2de76696b96a1cd828d2aa4d3ef9eb55.

              Show
              julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/a0ba73cd2de76696b96a1cd828d2aa4d3ef9eb55 .
              Hide
              Permalink
              julianhyde Julian Hyde added a comment -

              Closing now that 1.0.0-incubating has been released.

              Show
              julianhyde Julian Hyde added a comment - Closing now that 1.0.0-incubating has been released.

                People

                • Assignee:
                  julianhyde Julian Hyde
                  Reporter:
                  julianhyde Julian Hyde
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:

                    Development