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

          Activity

          Hide
          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.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/a0ba73cd2de76696b96a1cd828d2aa4d3ef9eb55 .
          Hide
          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
          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
          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
          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)

            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