New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

google_checks.xml CustomImportOrder problem #941

Closed
ctubbsii opened this Issue Apr 17, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@ctubbsii

ctubbsii commented Apr 17, 2015

The Google Java Style Guide specifies the import ordering to have com.google first, but only if it is in the com.google package. That rule seems to be enforced in google_checks.xml for everybody, even though the majority of people using these rules will probably not be writing code in that package.

This is a problem for people using these rules on their project, but importing Google code, like Guava. It seems like this would much more appropriately be applied with SAME_PACKAGE and a samePackageMatchingDepth of 2.

It's also not clear to me that the THIRD_PARTY_PACKAGE or STANDARD_JAVA_PACKAGE rule is correctly dividing things into separate groups "one group per top-level package". It seems CustomImportOrder needs an additional configuration option to separate THIRD_PARTY_PACKAGE into separate groups, and another flag to separate STANDARD_JAVA_PACKAGE into separate groups.

I'm not even sure there's a good workaround, short of copying/pasting the entire google_checks.xml file and manually modifying or deleting that rule. (I'm using the checkstyle-maven-plugin, so if anybody has any suggestions, along the lines of overriding only the CustomImportOrder rule directly in the pom.xml with the rest of the rules defined in the file, that'd be helpful).

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 19, 2015

Member

It seems like this would much more appropriately be applied with SAME_PACKAGE and a samePackageMatchingDepth of 2.

We created that option exactly for this, most likely we just fogot to use it when did configuration and guava report generation. We could use it but we need to recheck that guava report is the same.

It's also not clear to me that the THIRD_PARTY_PACKAGE or STANDARD_JAVA_PACKAGE rule is correctly dividing things into separate groups "one group per top-level package"

Please describe your concers in detail, i do not understand a problem. Standard packages are configurable, all thirdparty is all that does not match any other group.

I'm not even sure there's a good workaround, short of copying/pasting the entire google_checks.xml file and manually modifying or deleting that rule.

I always advice to use your own style, it is a rare case when rules of one team are ok for another team. We will deliver Google style with complete focus to their style guide, to show that our project could satisfy even demanding requirements.

with the rest of the rules defined in the file, that'd be helpful

There is no inheritance for configs, or using few configs.

Member

romani commented Apr 19, 2015

It seems like this would much more appropriately be applied with SAME_PACKAGE and a samePackageMatchingDepth of 2.

We created that option exactly for this, most likely we just fogot to use it when did configuration and guava report generation. We could use it but we need to recheck that guava report is the same.

It's also not clear to me that the THIRD_PARTY_PACKAGE or STANDARD_JAVA_PACKAGE rule is correctly dividing things into separate groups "one group per top-level package"

Please describe your concers in detail, i do not understand a problem. Standard packages are configurable, all thirdparty is all that does not match any other group.

I'm not even sure there's a good workaround, short of copying/pasting the entire google_checks.xml file and manually modifying or deleting that rule.

I always advice to use your own style, it is a rare case when rules of one team are ok for another team. We will deliver Google style with complete focus to their style guide, to show that our project could satisfy even demanding requirements.

with the rest of the rules defined in the file, that'd be helpful

There is no inheritance for configs, or using few configs.

@romani romani added the approved label Apr 19, 2015

@ctubbsii

This comment has been minimized.

Show comment
Hide comment
@ctubbsii

ctubbsii Apr 20, 2015

Please describe your concers in detail

Google defines (android.*, com.*, net.*, org.*, ..., java.*, javax.*) each as their own group, separated by one blank line between them. On the other hand, the THIRD_PARTY_PACKAGE would place most of these in a single group, while STANDARD_JAVA_PACKAGE would place java.* and javax.* in the same group. If placing each one of these into their own group is possible today (I don't think it is in the current CustomImportOrder), then the google_checks.xml is not correctly doing that.

(Personally, I think all this grouping is kinda dumb for the Google Style. I'd rather just: alphabetized static imports, blank line, alphabetized non-static imports, but unfortunately that's not what the Google Style Guide has chosen, so it makes checkstyle enforcement a lot more complicated.)

I always advice to use your own style, it is a rare case when rules of one team are ok for another team.

I suspect many projects find it easier to adopt one of the available standards rather than write their own, especially since the checkstyle-maven-plugin makes the built-in styles in checkstyle available for direct use by name, and especially if the standard is well documented, reasonable, and IDE-independent, as is the case for Google's style.

We will deliver Google style with complete focus to their style guide, to show that our project could satisfy even demanding requirements.

And for the most part, it does so successfully. I think imports are one of those few areas where there remains a (slight) mismatch between the requirements and the abilities of checkstyle.

But also, please keep in mind that Checkstyle's users don't necessarily just use it just as an example. It's actually a reasonable way to enforce a standard which users may adopt.

There is no inheritance for configs, or using few configs.

Thanks. The ability to override might be a feature I'd want to take up with the checkstyle-maven-plugin.

Please describe your concers in detail

Google defines (android.*, com.*, net.*, org.*, ..., java.*, javax.*) each as their own group, separated by one blank line between them. On the other hand, the THIRD_PARTY_PACKAGE would place most of these in a single group, while STANDARD_JAVA_PACKAGE would place java.* and javax.* in the same group. If placing each one of these into their own group is possible today (I don't think it is in the current CustomImportOrder), then the google_checks.xml is not correctly doing that.

(Personally, I think all this grouping is kinda dumb for the Google Style. I'd rather just: alphabetized static imports, blank line, alphabetized non-static imports, but unfortunately that's not what the Google Style Guide has chosen, so it makes checkstyle enforcement a lot more complicated.)

I always advice to use your own style, it is a rare case when rules of one team are ok for another team.

I suspect many projects find it easier to adopt one of the available standards rather than write their own, especially since the checkstyle-maven-plugin makes the built-in styles in checkstyle available for direct use by name, and especially if the standard is well documented, reasonable, and IDE-independent, as is the case for Google's style.

We will deliver Google style with complete focus to their style guide, to show that our project could satisfy even demanding requirements.

And for the most part, it does so successfully. I think imports are one of those few areas where there remains a (slight) mismatch between the requirements and the abilities of checkstyle.

But also, please keep in mind that Checkstyle's users don't necessarily just use it just as an example. It's actually a reasonable way to enforce a standard which users may adopt.

There is no inheritance for configs, or using few configs.

Thanks. The ability to override might be a feature I'd want to take up with the checkstyle-maven-plugin.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 21, 2015

Member

Google defines (android., com., net., org., ..., java., javax.) each as their own group, separated by one blank line between them. On the other hand, the THIRD_PARTY_PACKAGE would place most of these in a single group,

Are you sure ?
We know that Google does not manage its import manually , we create a config that does not do any violation on their code, and have one more prove that we on right way of interpreting their rules.
Current report - http://checkstyle.sourceforge.net/reports/google-style/guava/

Personally, I think all this grouping is kinda dumb for the Google Style

here you are describe your preferences and your vision of style. It is good , but here we follow strict document requirements. But by experience of discussion with Guava team I got to know that even Google's is not defined clearly and a lot have to be changed at document. Please read - google/guava#1891

I suspect many projects find it easier to adopt one of the available standards rather than write their own

yes, it is good, but Google use only third part of all static analysis that our tool provide. Following to Google is better then nothing but eventually you will change a config.

I think imports are one of those few areas where there remains a (slight) mismatch between the requirements and the abilities of checkstyle.

we could sort imports string :) , we might not be able to satisfy your requirements, but this is another discussion.

please provide a config that you would like to have that covers Google and satisfy you (with SAME_PACKAGE usage), please test your new config as described there https://github.com/checkstyle/checkstyle/wiki/How-to-generate-Checkstyle-report-for-Google-Guava-project and prove that change does not generate any violations over guava project.

Member

romani commented Apr 21, 2015

Google defines (android., com., net., org., ..., java., javax.) each as their own group, separated by one blank line between them. On the other hand, the THIRD_PARTY_PACKAGE would place most of these in a single group,

Are you sure ?
We know that Google does not manage its import manually , we create a config that does not do any violation on their code, and have one more prove that we on right way of interpreting their rules.
Current report - http://checkstyle.sourceforge.net/reports/google-style/guava/

Personally, I think all this grouping is kinda dumb for the Google Style

here you are describe your preferences and your vision of style. It is good , but here we follow strict document requirements. But by experience of discussion with Guava team I got to know that even Google's is not defined clearly and a lot have to be changed at document. Please read - google/guava#1891

I suspect many projects find it easier to adopt one of the available standards rather than write their own

yes, it is good, but Google use only third part of all static analysis that our tool provide. Following to Google is better then nothing but eventually you will change a config.

I think imports are one of those few areas where there remains a (slight) mismatch between the requirements and the abilities of checkstyle.

we could sort imports string :) , we might not be able to satisfy your requirements, but this is another discussion.

please provide a config that you would like to have that covers Google and satisfy you (with SAME_PACKAGE usage), please test your new config as described there https://github.com/checkstyle/checkstyle/wiki/How-to-generate-Checkstyle-report-for-Google-Guava-project and prove that change does not generate any violations over guava project.

@ctubbsii

This comment has been minimized.

Show comment
Hide comment
@ctubbsii

ctubbsii Apr 21, 2015

Are you sure ?

I'm sure about the Google Java Style description for imports, yes. The requirements are quite clearly documented. I'm less confident about CustomImportOrder behavior, because my testing with various configuration options is limited.

here you are describe your preferences and your vision of style.

Yes, apologies for any confusion. That was intended solely as a parenthetical comment, an aside. I was merely expressing a slight frustration with Google's choices to have such complex import requirements.

we might not be able to satisfy your requirements

To be clear, the import order requirements I'm describing in this issue are from the Google Style Guide for Java, and not my own.

FWIW, Guava isn't really a very good test for the CustomImportOrder rule. That library explicitly avoids third-party imports, and I don't believe it has any javax.* imports either.

Are you sure ?

I'm sure about the Google Java Style description for imports, yes. The requirements are quite clearly documented. I'm less confident about CustomImportOrder behavior, because my testing with various configuration options is limited.

here you are describe your preferences and your vision of style.

Yes, apologies for any confusion. That was intended solely as a parenthetical comment, an aside. I was merely expressing a slight frustration with Google's choices to have such complex import requirements.

we might not be able to satisfy your requirements

To be clear, the import order requirements I'm describing in this issue are from the Google Style Guide for Java, and not my own.

FWIW, Guava isn't really a very good test for the CustomImportOrder rule. That library explicitly avoids third-party imports, and I don't believe it has any javax.* imports either.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 22, 2015

Member

looks like we did not noticed "one group per top-level package" from "Third-party imports, one group per top-level package, in ASCII sort order for example: android, com, junit, org, sun"

Examples for muli-thirdparty and javax:
https://github.com/google/guava/blob/master/guava-tests/test/com/google/common/collect/ImmutableSortedMultisetTest.java
https://github.com/google/guava/blob/master/guava/src/com/google/common/io/Closer.java

We need to investigate why Check we have one macros for thirdparty and one option separateLineBetweenGroups and it apply to imports inside one group too - it is not obvious so it is bad design. Even in style it is stated as "Within a group there are no blank lines....." .
We also need to rethink why we have "java" and "javax" in the same group , and Google is describing them as separate groups.

here is a beginning of discussion with original author: https://groups.google.com/d/msg/checkstyle-devel/E0z89fzvxGs/li4gPrstLZAJ it might be useful to understand why we did that in such way.

Member

romani commented Apr 22, 2015

looks like we did not noticed "one group per top-level package" from "Third-party imports, one group per top-level package, in ASCII sort order for example: android, com, junit, org, sun"

Examples for muli-thirdparty and javax:
https://github.com/google/guava/blob/master/guava-tests/test/com/google/common/collect/ImmutableSortedMultisetTest.java
https://github.com/google/guava/blob/master/guava/src/com/google/common/io/Closer.java

We need to investigate why Check we have one macros for thirdparty and one option separateLineBetweenGroups and it apply to imports inside one group too - it is not obvious so it is bad design. Even in style it is stated as "Within a group there are no blank lines....." .
We also need to rethink why we have "java" and "javax" in the same group , and Google is describing them as separate groups.

here is a beginning of discussion with original author: https://groups.google.com/d/msg/checkstyle-devel/E0z89fzvxGs/li4gPrstLZAJ it might be useful to understand why we did that in such way.

@ivanov-alex

This comment has been minimized.

Show comment
Hide comment
@ivanov-alex

ivanov-alex Sep 15, 2015

Contributor

So there are 2 requests:

  1. Changing 'com.google' from SPECIAL to SAME_PACKAGE
  2. Adding an option to check line separator for "one group per top-level package"

Regarding _#_1:

  • It will not 100% match Google Style guide as it is says com.google imports (only if this source file is in the com.google package space) and has nothing specified for other package space. And one of the Google projects (Bazel) that has files in com.facebook package space does not put these imports right after static: example1, example2
  • In addition, I haven't seen an option similar to SAME_PACKAGE on Eclipse, Idea, NetBeans. You have to specify groups as regular expressions. So IDEs will organize imports in the same order regardless of a package when you have multiple package spaces in one project. It will conflict with Checkstyle and require manual correction of imports.
  • On the other hand, I think that sorting imports which are native for the project separately from other imports is useful and SAME_PACKAGE is great option for projects that has only one package space.

For _#_2 I see 2 options:
A) Introduce new option that will force check for separator between top-level domains in one group. Problems are:

  • According to Google Style guide, it is required for third-party and standard, but not for static. So it looks like we will need to make an option for every group.
  • You will not be able to configure group-per-top-domain option on IDE configuration: if you want to have some imports on your code visually separated as a group, you have to manually add a group mask. Basically, it means that every time you add import from new top-level domain to your source code, you have to add new group on IDE configuration (otherwise Checkstyle will raise violation).

B) Provide ability for user to specify more groups on CustomImportOrder configuration. It will require manual configuration every time when new top-level domain is added to source code (and the same configuration should be done on IDE, see previous comment).
Considering problems with option A, I would recommend option B. Option B is not perfect (require more work for configuration), but it works similarly to IDE configuration (and you still have to do configurat). sometimes we already feel limitation with current amount of groups supported by CustomImportOrder and ability to introduce custom groups will make the check more flexible.

@romani Could you please comment with your vision?

Contributor

ivanov-alex commented Sep 15, 2015

So there are 2 requests:

  1. Changing 'com.google' from SPECIAL to SAME_PACKAGE
  2. Adding an option to check line separator for "one group per top-level package"

Regarding _#_1:

  • It will not 100% match Google Style guide as it is says com.google imports (only if this source file is in the com.google package space) and has nothing specified for other package space. And one of the Google projects (Bazel) that has files in com.facebook package space does not put these imports right after static: example1, example2
  • In addition, I haven't seen an option similar to SAME_PACKAGE on Eclipse, Idea, NetBeans. You have to specify groups as regular expressions. So IDEs will organize imports in the same order regardless of a package when you have multiple package spaces in one project. It will conflict with Checkstyle and require manual correction of imports.
  • On the other hand, I think that sorting imports which are native for the project separately from other imports is useful and SAME_PACKAGE is great option for projects that has only one package space.

For _#_2 I see 2 options:
A) Introduce new option that will force check for separator between top-level domains in one group. Problems are:

  • According to Google Style guide, it is required for third-party and standard, but not for static. So it looks like we will need to make an option for every group.
  • You will not be able to configure group-per-top-domain option on IDE configuration: if you want to have some imports on your code visually separated as a group, you have to manually add a group mask. Basically, it means that every time you add import from new top-level domain to your source code, you have to add new group on IDE configuration (otherwise Checkstyle will raise violation).

B) Provide ability for user to specify more groups on CustomImportOrder configuration. It will require manual configuration every time when new top-level domain is added to source code (and the same configuration should be done on IDE, see previous comment).
Considering problems with option A, I would recommend option B. Option B is not perfect (require more work for configuration), but it works similarly to IDE configuration (and you still have to do configurat). sometimes we already feel limitation with current amount of groups supported by CustomImportOrder and ability to introduce custom groups will make the check more flexible.

@romani Could you please comment with your vision?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 22, 2015

Member

@ivanov-alex ,

  1. Changing 'com.google' from SPECIAL to SAME_PACKAGE

I tested on Guava "STATIC###SAME_PACKAGE(2)###THIRD_PARTY_PACKAGE###STANDARD_JAVA_PACKAGE" vs "STATIC###SPECIAL_IMPORTS###THIRD_PARTY_PACKAGE###STANDARD_JAVA_PACKAGE", both give a 0 violations.
so we are ok to change this because:

  • there is no guaranty that Google specified their rules correctly, it is know issue that not all team in Google agree on all rules, see discussion at google/guava#1891 (comment)
  • base on first, we need to make config usable for java community. Google do not use Checkstyle, and do not have time to update its Style guide. So lets focus on team who use us. When Google come to me with request to update config to his specification, we will discuss with it all details of ordering.
  • SAME_PACKAGE(2) usage is ok to my mind, and users will be able to adjust IDEs appropriately. "com.mycompany" at their IDE settings - will work in most cases.

And one of the Google projects (Bazel) that has files in com.facebook package space does not put these imports right after static

I do believe that such code is not formatted well to style guide, and as Checkstyle is not used in build - it is very easy to miss/forget.... .

Member

romani commented Sep 22, 2015

@ivanov-alex ,

  1. Changing 'com.google' from SPECIAL to SAME_PACKAGE

I tested on Guava "STATIC###SAME_PACKAGE(2)###THIRD_PARTY_PACKAGE###STANDARD_JAVA_PACKAGE" vs "STATIC###SPECIAL_IMPORTS###THIRD_PARTY_PACKAGE###STANDARD_JAVA_PACKAGE", both give a 0 violations.
so we are ok to change this because:

  • there is no guaranty that Google specified their rules correctly, it is know issue that not all team in Google agree on all rules, see discussion at google/guava#1891 (comment)
  • base on first, we need to make config usable for java community. Google do not use Checkstyle, and do not have time to update its Style guide. So lets focus on team who use us. When Google come to me with request to update config to his specification, we will discuss with it all details of ordering.
  • SAME_PACKAGE(2) usage is ok to my mind, and users will be able to adjust IDEs appropriately. "com.mycompany" at their IDE settings - will work in most cases.

And one of the Google projects (Bazel) that has files in com.facebook package space does not put these imports right after static

I do believe that such code is not formatted well to style guide, and as Checkstyle is not used in build - it is very easy to miss/forget.... .

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 22, 2015

Member
  1. Adding an option to check line separator for "one group per top-level package"

According to Google Style guide, it is required for third-party and standard, but not for static. So it looks like we will need to make an option for every group

We could describe in documentation that option is applicable to THIRDPARTY and STANDARD groups only. It is better then deprecate existing option and provide two new options.
I presume we already do separation and problem only in documentation.

You will not be able to configure group-per-top-domain option on IDE configuration: if you want to have some imports on your code visually separated as a group, you have to manually add a group mask. Basically, it means that every time you add import from new top-level domain to your source code, you have to add new group on IDE configuration (otherwise Checkstyle will raise violation).

I thought it is possible to make config like following to avoid problems you describe.
Eclipse:

*(static)
com.google
*
java
javax

IntelijIdea:

import static all other imports
<blank line>
import com.google.*
<blank line>
import all other imports
<blank line>
import java.*
<blank line>
import javax.*
Member

romani commented Sep 22, 2015

  1. Adding an option to check line separator for "one group per top-level package"

According to Google Style guide, it is required for third-party and standard, but not for static. So it looks like we will need to make an option for every group

We could describe in documentation that option is applicable to THIRDPARTY and STANDARD groups only. It is better then deprecate existing option and provide two new options.
I presume we already do separation and problem only in documentation.

You will not be able to configure group-per-top-domain option on IDE configuration: if you want to have some imports on your code visually separated as a group, you have to manually add a group mask. Basically, it means that every time you add import from new top-level domain to your source code, you have to add new group on IDE configuration (otherwise Checkstyle will raise violation).

I thought it is possible to make config like following to avoid problems you describe.
Eclipse:

*(static)
com.google
*
java
javax

IntelijIdea:

import static all other imports
<blank line>
import com.google.*
<blank line>
import all other imports
<blank line>
import java.*
<blank line>
import javax.*
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 24, 2015

Member

Interesting examples of google's code - https://github.com/android/platform_development/tree/master/ide

IntelijIdea config is not following Style guide (static are below, no alphabetical order) - https://github.com/android/platform_development/blob/master/ide/intellij/codestyles/AndroidStyle.xml#L19 , BUT we see how "com.google" is separated to the top as it demand style guide.

Eclipse config - https://github.com/android/platform_development/blob/master/ide/eclipse/android.importorder - but here there is no reference to special "com.google" at all (most likely android does not have sources from at "com/google" folders)

Alternative proposal from @ivanov-alex is to use http://checkstyle.sourceforge.net/config_imports.html#ImportOrder that is designed the same way as IDEs formatters. In config just specify most common domain of first level from https://repo1.maven.org/maven2/ , or grab list from Eclipse settings of Android project (see link above).

Chromium project does not use Google Style config that we host and use its own - https://code.google.com/p/chromium/codesearch#chromium/src/tools/android/checkstyle/chromium-style-5.0.xml&l=164

Member

romani commented Sep 24, 2015

Interesting examples of google's code - https://github.com/android/platform_development/tree/master/ide

IntelijIdea config is not following Style guide (static are below, no alphabetical order) - https://github.com/android/platform_development/blob/master/ide/intellij/codestyles/AndroidStyle.xml#L19 , BUT we see how "com.google" is separated to the top as it demand style guide.

Eclipse config - https://github.com/android/platform_development/blob/master/ide/eclipse/android.importorder - but here there is no reference to special "com.google" at all (most likely android does not have sources from at "com/google" folders)

Alternative proposal from @ivanov-alex is to use http://checkstyle.sourceforge.net/config_imports.html#ImportOrder that is designed the same way as IDEs formatters. In config just specify most common domain of first level from https://repo1.maven.org/maven2/ , or grab list from Eclipse settings of Android project (see link above).

Chromium project does not use Google Style config that we host and use its own - https://code.google.com/p/chromium/codesearch#chromium/src/tools/android/checkstyle/chromium-style-5.0.xml&l=164

@floscher

This comment has been minimized.

Show comment
Hide comment
@floscher

floscher Jul 21, 2016

Nine days ago the guidelines for import formatting have been simplified drastically: google/styleguide@b075cb7

Now the only requirements are that static imports come first, then all other imports in one group. These two groups are separated by an empty line (if both groups are present). And both groups are still sorted alphabetically in ASCII order.

I prepared a pull request to reflect for this change: #3363.

Nine days ago the guidelines for import formatting have been simplified drastically: google/styleguide@b075cb7

Now the only requirements are that static imports come first, then all other imports in one group. These two groups are separated by an empty line (if both groups are present). And both groups are still sorted alphabetically in ASCII order.

I prepared a pull request to reflect for this change: #3363.

MEZk added a commit to MEZk/checkstyle that referenced this issue Oct 30, 2016

MEZk added a commit to MEZk/checkstyle that referenced this issue Oct 30, 2016

romani added a commit that referenced this issue Nov 5, 2016

@romani romani added the new feature label Nov 5, 2016

@romani romani added this to the 7.3 milestone Nov 5, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 5, 2016

Member

fix is merged

Member

romani commented Nov 5, 2016

fix is merged

@romani romani closed this Nov 5, 2016

GalateaEric added a commit to GalateaRaj/fuse-settings that referenced this issue Dec 18, 2017

effectively removing the import ordering for fuse. Checkstyle use to …
…have certain rules around com.google.* packages coming first, but according to checkstyle/checkstyle#941 the checkstyle people re-interpreted the meaning of that particular part of the google java style guide.  Eclipse will effectively default to lexicographic order which is what the google_checks.xml looks for.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment