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

ImportControl should have property which allows to validate specified file path #3462

Closed
MEZk opened this Issue Sep 23, 2016 · 31 comments

Comments

Projects
None yet
4 participants
@MEZk
Contributor

MEZk commented Sep 23, 2016

ImportControl should have parameter (atribute for <import-control> element) which allows user to force the check to validate only classes which are in the specific path. Proposal attribute name is "path". Attribute type should be regexp pattern.

Example of how to configure the check to validate only classes which are located in com.puppycrawl.tools.checkstyle package and which file path matches
the regexp ^.*[\\/]main[\\/].*$.
Proposal:

<import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]main[\\/].*$">
    ...
    <allow class="com.google.common.base.CaseFormat" local-only="true"/>
    ...
</import-control>

In documentation we need to show how to make two configurations for main and test areas of code.

In scope of this issue we need to split configuration into two and make rules for main code more strict

This will allow us to restrict usage of Guava library (#3433) in main code and keep freedom to use convenient methods in test area.

UPDATE:
final implementation made config be like:

     <module name="ImportControl">
       <property name="file" value="${importcontrol.file}"/>
       <property name="path" value="^.*[\\/]src[\\/]main[\\/].*$"/>
     </module>

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@MEZk MEZk changed the title from ImportControl should have parameter which allows to validate specified scope: test or main to ImportControl should have parameter which allows to validate specified file path Sep 23, 2016

@romani romani added the approved label Sep 27, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 27, 2016

Member

path should be regexp type by default, as there are not reasons to use it as plain text equals pattern.

Note: all other attributes are following regex attribute to switch mode from "equals" to "match". I do not see any application of path in model of "equals".

Member

romani commented Sep 27, 2016

path should be regexp type by default, as there are not reasons to use it as plain text equals pattern.

Note: all other attributes are following regex attribute to switch mode from "equals" to "match". I do not see any application of path in model of "equals".

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 3, 2016

Contributor

I will implement this.

Contributor

jochenvdv commented Dec 3, 2016

I will implement this.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 3, 2016

Contributor

I will have to make a new version (1.3) of the import control DTD, correct?
I have to reference it from the new import control configuration I'm making for testing, so should it be done separately and deployed to the site first?

Contributor

jochenvdv commented Dec 3, 2016

I will have to make a new version (1.3) of the import control DTD, correct?
I have to reference it from the new import control configuration I'm making for testing, so should it be done separately and deployed to the site first?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 3, 2016

Member

Can you deploy it your github.io or just reference github raw file content from your fork?
This will give you to proceed.
As we come close merge , we will split new version to sepatate PR and i put it to our static web site

Member

romani commented Dec 3, 2016

Can you deploy it your github.io or just reference github raw file content from your fork?
This will give you to proceed.
As we come close merge , we will split new version to sepatate PR and i put it to our static web site

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 3, 2016

Contributor

@romani I will do that.

Contributor

jochenvdv commented Dec 3, 2016

@romani I will do that.

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Dec 4, 2016

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 5, 2016

Contributor

This functionality could be achieved by adding a BeforeExecutionExclusionFileFilter to the Checker, but this would require multiple configurations.
I think adding this attribute on ImportControlCheck is a good idea but I'm not sure about how to implement this. It seems that excluding files is not really a responsability for Checks.

Should I skip over files that don't match the pattern in ImportControlCheck? This could be done by getFileContents().getFileName(), but this method returns the full absolute path. Is there a way to find the base path for Checkstyle's execution from within an AbstractCheck so that it can be stripped of before being matched by the pattern (I assume the pattern should be relative to the base path)?

I would do the matching in beginTree and set a boolean indicating whether the file should be skipped. Then, visitToken would do nothing depending on that variable. Does this sound good?

It would be nice if beginTree could return a boolean so TreeWalker could skip calling visitToken unnecessarily.

Contributor

jochenvdv commented Dec 5, 2016

This functionality could be achieved by adding a BeforeExecutionExclusionFileFilter to the Checker, but this would require multiple configurations.
I think adding this attribute on ImportControlCheck is a good idea but I'm not sure about how to implement this. It seems that excluding files is not really a responsability for Checks.

Should I skip over files that don't match the pattern in ImportControlCheck? This could be done by getFileContents().getFileName(), but this method returns the full absolute path. Is there a way to find the base path for Checkstyle's execution from within an AbstractCheck so that it can be stripped of before being matched by the pattern (I assume the pattern should be relative to the base path)?

I would do the matching in beginTree and set a boolean indicating whether the file should be skipped. Then, visitToken would do nothing depending on that variable. Does this sound good?

It would be nice if beginTree could return a boolean so TreeWalker could skip calling visitToken unnecessarily.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 5, 2016

Member

Should I skip over files that don't match the pattern in ImportControlCheck?

Yes, it is no different than AbstractFileSets which choose any specific files to look at, example TranslationCheck. It just needs to be documented to user.

This could be done by getFileContents().getFileName(), but this method returns the full absolute path.

This is what we usually work with, this is why the pattern is regular expression and must be a very distinct area. See OuterTypeFilenameCheck.
Checker does have basedir, but user may not always set it and I'm not sure if we can currently reach it from checks. It is more on other utilities to set this (maven) since everyone puts their workspaces in different areas. Currently I see no checks using it. It is just for violations, but it could be a good idea to hook into it for our checks.

I would do the matching in beginTree and set a boolean indicating whether the file should be skipped. Then, visitToken would do nothing depending on that variable. Does this sound good?

It sounds good to me as long as the requirements don't change.


Just my thoughts on this issue.

In scope of this issue we need to split configuration into two

@romani I am against 2 configuration files for various reasons. This will double the amount of time a full run will take since we don't support 2 configurations in 1 run (double listing files, double parsing, etc). It is also more tenuous to keep 2 different configurations in synch, especially if users want same checks in both areas, like us. This will also require our cache to be split into 2 files to avoid conflicts.

Something like this would increase the burden on us and other users for maintainability, imo.

Is it not reasonable to set the file path either on the allow/disallow tag or have its own tag?
Examples:

<import-control pkg="com.puppycrawl.tools.checkstyle">
    ...
    <allow class="com.google.common.base.CaseFormat" local-only="true" files="^.*[\\/]main[\\/].*$"/>
    ...
</import-control>

or

<import-control pkg="com.puppycrawl.tools.checkstyle">
    ...
    <files regexp="^.*[\\/]main[\\/].*$">
        <allow class="com.google.common.base.CaseFormat" local-only="true"/>
    </files>
    ...
</import-control>
Member

rnveach commented Dec 5, 2016

Should I skip over files that don't match the pattern in ImportControlCheck?

Yes, it is no different than AbstractFileSets which choose any specific files to look at, example TranslationCheck. It just needs to be documented to user.

This could be done by getFileContents().getFileName(), but this method returns the full absolute path.

This is what we usually work with, this is why the pattern is regular expression and must be a very distinct area. See OuterTypeFilenameCheck.
Checker does have basedir, but user may not always set it and I'm not sure if we can currently reach it from checks. It is more on other utilities to set this (maven) since everyone puts their workspaces in different areas. Currently I see no checks using it. It is just for violations, but it could be a good idea to hook into it for our checks.

I would do the matching in beginTree and set a boolean indicating whether the file should be skipped. Then, visitToken would do nothing depending on that variable. Does this sound good?

It sounds good to me as long as the requirements don't change.


Just my thoughts on this issue.

In scope of this issue we need to split configuration into two

@romani I am against 2 configuration files for various reasons. This will double the amount of time a full run will take since we don't support 2 configurations in 1 run (double listing files, double parsing, etc). It is also more tenuous to keep 2 different configurations in synch, especially if users want same checks in both areas, like us. This will also require our cache to be split into 2 files to avoid conflicts.

Something like this would increase the burden on us and other users for maintainability, imo.

Is it not reasonable to set the file path either on the allow/disallow tag or have its own tag?
Examples:

<import-control pkg="com.puppycrawl.tools.checkstyle">
    ...
    <allow class="com.google.common.base.CaseFormat" local-only="true" files="^.*[\\/]main[\\/].*$"/>
    ...
</import-control>

or

<import-control pkg="com.puppycrawl.tools.checkstyle">
    ...
    <files regexp="^.*[\\/]main[\\/].*$">
        <allow class="com.google.common.base.CaseFormat" local-only="true"/>
    </files>
    ...
</import-control>
@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 5, 2016

Contributor

@rnveach Thanks for answering my questions. I will will wait on @romani's input on the multiple configurations before I continue.

Contributor

jochenvdv commented Dec 5, 2016

@rnveach Thanks for answering my questions. I will will wait on @romani's input on the multiple configurations before I continue.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 6, 2016

Member

Should I skip over files that don't match the pattern in ImportControlCheck?

yes.

Is there a way to find the base path for Checkstyle's execution from within an AbstractCheck so that it can be stripped of before being matched by the pattern (I assume the pattern should be relative to the base path)?

no way, it will be a problem for mode when checkstyle is running in IDE.

I would do the matching in beginTree and set a boolean indicating whether the file should be skipped. Then, visitToken would do nothing depending on that variable. Does this sound good?

yes. Check should do all to optimize his performance.

It would be nice if beginTree could return a boolean so TreeWalker could skip calling visitToken unnecessarily.

No, that is change in API and not really required. beginTree --> false - mean nothing. That method is event, what you/Check do with event is custom Check business.

I am against 2 configuration files for various reasons.

yes, I am against too.

It is better to have two import-control tags in the same file (not possible now , but we might play with DTD to allow this).
If not possible we should consider update of all child tags with new attribute path.
Before any implementation , please make a mock configs for Checkstyle's import-control.xml to see that will looks like for both variants and lets decide what it better approach together.

Member

romani commented Dec 6, 2016

Should I skip over files that don't match the pattern in ImportControlCheck?

yes.

Is there a way to find the base path for Checkstyle's execution from within an AbstractCheck so that it can be stripped of before being matched by the pattern (I assume the pattern should be relative to the base path)?

no way, it will be a problem for mode when checkstyle is running in IDE.

I would do the matching in beginTree and set a boolean indicating whether the file should be skipped. Then, visitToken would do nothing depending on that variable. Does this sound good?

yes. Check should do all to optimize his performance.

It would be nice if beginTree could return a boolean so TreeWalker could skip calling visitToken unnecessarily.

No, that is change in API and not really required. beginTree --> false - mean nothing. That method is event, what you/Check do with event is custom Check business.

I am against 2 configuration files for various reasons.

yes, I am against too.

It is better to have two import-control tags in the same file (not possible now , but we might play with DTD to allow this).
If not possible we should consider update of all child tags with new attribute path.
Before any implementation , please make a mock configs for Checkstyle's import-control.xml to see that will looks like for both variants and lets decide what it better approach together.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 9, 2016

Contributor

@romani OK, I will make example configs based on Checkstyle's import-control.xml, that will demonstrate disallowing Guava only in main directory, by following methods:

  1. Attribute on allow/disallow element
  2. Separate element to group allow/disallow by path pattern (like @rnveach's example)
  3. Separate import-control tags (Please explain? XML should have one root element)

Regarding the name for the attribute/tag, I think path is the best choice for option 1, and files is the better name for option 2.

Contributor

jochenvdv commented Dec 9, 2016

@romani OK, I will make example configs based on Checkstyle's import-control.xml, that will demonstrate disallowing Guava only in main directory, by following methods:

  1. Attribute on allow/disallow element
  2. Separate element to group allow/disallow by path pattern (like @rnveach's example)
  3. Separate import-control tags (Please explain? XML should have one root element)

Regarding the name for the attribute/tag, I think path is the best choice for option 1, and files is the better name for option 2.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 11, 2016

Member

I will make example configs based

please do no go crazy with, we just need mock view (it should not work).

Separate import-control tags (Please explain? XML should have one root element)

Yes, root element is only one by DTD - https://www.w3.org/TR/REC-xml/#dt-root
but we can change dtd to introduce new root element, we will need to bump versions in any case.
Example of full freedom in test.

<some-root>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="src/test/java">
  </import-control>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="src/main/java">

    <allow pkg="antlr"/>
    <allow pkg="org.antlr.v4.runtime"/>
     .....
  </import-control>
<some-root>

I think path is the best choice for option 1, and files is the better name for option 2.

Lets have a look.
BUT Mixing rules for production code and tests in the same file(line located close to each other) is not good and error prone.

Member

romani commented Dec 11, 2016

I will make example configs based

please do no go crazy with, we just need mock view (it should not work).

Separate import-control tags (Please explain? XML should have one root element)

Yes, root element is only one by DTD - https://www.w3.org/TR/REC-xml/#dt-root
but we can change dtd to introduce new root element, we will need to bump versions in any case.
Example of full freedom in test.

<some-root>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="src/test/java">
  </import-control>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="src/main/java">

    <allow pkg="antlr"/>
    <allow pkg="org.antlr.v4.runtime"/>
     .....
  </import-control>
<some-root>

I think path is the best choice for option 1, and files is the better name for option 2.

Lets have a look.
BUT Mixing rules for production code and tests in the same file(line located close to each other) is not good and error prone.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 16, 2016

Contributor

I have quickly made some example configurations based on config/import-control.xml in this Gist demonstrating how Guava could be allowed only in the src/test directory. I am not sure if I did this correctly, feedback is welcome.

Personally I prefer having a files element, but creating a new root element could prove more flexible. A path attribute on allow/disallow elements would create too much repetition.

Contributor

jochenvdv commented Dec 16, 2016

I have quickly made some example configurations based on config/import-control.xml in this Gist demonstrating how Guava could be allowed only in the src/test directory. I am not sure if I did this correctly, feedback is welcome.

Personally I prefer having a files element, but creating a new root element could prove more flexible. A path attribute on allow/disallow elements would create too much repetition.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 16, 2016

Member

your confis in short:
1)

<import-control pkg="com.puppycrawl.tools.checkstyle">
  <!-- Allow Guava only in src/test directory -->
  <files pattern="^.*[\\/]src[\\/]test[\\/].*$">
      <allow pkg="com.google.common"/>
</files>
<new-root>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]src[\\/]test[\\/].*$">
    <!-- Full freedom in src/test directory -->
  </import-control>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]src[\\/]main[\\/].*$">
    <!-- strict rules -->
  </import-control>
<import-control pkg="com.puppycrawl.tools.checkstyle">
  <!-- Allow Guava only in src/test directory -->
<allow pkg="com.google.common" path="^.*[\\/]src[\\/]test[\\/].*$"/>

I do not like 1) it looks confusing. 2) and 3) could be clear without documentation.
My vote is to use 2) , I see that mixing config for production code and testing in the same place is error prone. The bigger distance between them the better (ideally different files, but ...).

@rnveach , please share your ideas

Member

romani commented Dec 16, 2016

your confis in short:
1)

<import-control pkg="com.puppycrawl.tools.checkstyle">
  <!-- Allow Guava only in src/test directory -->
  <files pattern="^.*[\\/]src[\\/]test[\\/].*$">
      <allow pkg="com.google.common"/>
</files>
<new-root>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]src[\\/]test[\\/].*$">
    <!-- Full freedom in src/test directory -->
  </import-control>
  <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]src[\\/]main[\\/].*$">
    <!-- strict rules -->
  </import-control>
<import-control pkg="com.puppycrawl.tools.checkstyle">
  <!-- Allow Guava only in src/test directory -->
<allow pkg="com.google.common" path="^.*[\\/]src[\\/]test[\\/].*$"/>

I do not like 1) it looks confusing. 2) and 3) could be clear without documentation.
My vote is to use 2) , I see that mixing config for production code and testing in the same place is error prone. The bigger distance between them the better (ideally different files, but ...).

@rnveach , please share your ideas

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 16, 2016

Contributor

2 is OK, but @rnveach's early idea is also good:

<import-control pkg="com.puppycrawl.tools.checkstyle">
    ...
    <allow class="com.google.common.base.CaseFormat" local-only="true" files="^.*[\\/]main[\\/].*$"/>
    ...
</import-control>
Contributor

MEZk commented Dec 16, 2016

2 is OK, but @rnveach's early idea is also good:

<import-control pkg="com.puppycrawl.tools.checkstyle">
    ...
    <allow class="com.google.common.base.CaseFormat" local-only="true" files="^.*[\\/]main[\\/].*$"/>
    ...
</import-control>
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 16, 2016

Member

@MEZk , please rank ideas to what you would like to have .

Member

romani commented Dec 16, 2016

@MEZk , please rank ideas to what you would like to have .

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 22, 2016

Member

@romani
2 looks misleading. It does not define anywhere which one is test and which one is production except in a comment. I will assume the main tag for test should be <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]test[\\/].*$"> and the main tag for production is <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]main[\\/].*$">.

I give 1 and 3 the same level of approval. To me they are the same as a blocked if versus a non-blocked if in regards to the number of statements being surrounded. if (condition) statement; versus if (condition) { statement1; .... }. 1 does stand out more in terms of quick reading.

In terms of ranking between 1/3 and 2, to me it really depends on how dramatically different the list of imports are. I know you like separating test from production, but is the differences between the 2 configs going to be that big? Right now, only complaint is guava and I am only seeing this once in the examples given. If everything else will be the same between the 2, then it puts more work on us to keep them in synch. So for our current use, I would rank 1/3 as higher.

I do have one additional option to present that I didn't think of before. We'll call this option 4.
This whole problem is stemming from the fact we only allow one import control check in one config. We allow multiple instances of the same check for others with different properties like NoWhitespaceBefore.
We could take the files/path attribute and move it to the check and separate test and production that way. This is similar to option 2, but would allow the files to be completely separated.
Example:

    <module name="ImportControl">
      <property name="file" value="import-control-test.xml"/>
      <property name="scanPath" value="^.*[\\/]test[\\/].*$"/>
    </module>
    <module name="ImportControl">
      <property name="file" value="import-control-main.xml"/>
      <property name="scanPath" value="^.*[\\/]main[\\/].*$"/>
    </module>

Terms of ranking, I still group this with 2 and but see it as slightly higher than 2.

Final results: 1/3, 4, 2.

Member

rnveach commented Dec 22, 2016

@romani
2 looks misleading. It does not define anywhere which one is test and which one is production except in a comment. I will assume the main tag for test should be <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]test[\\/].*$"> and the main tag for production is <import-control pkg="com.puppycrawl.tools.checkstyle" path="^.*[\\/]main[\\/].*$">.

I give 1 and 3 the same level of approval. To me they are the same as a blocked if versus a non-blocked if in regards to the number of statements being surrounded. if (condition) statement; versus if (condition) { statement1; .... }. 1 does stand out more in terms of quick reading.

In terms of ranking between 1/3 and 2, to me it really depends on how dramatically different the list of imports are. I know you like separating test from production, but is the differences between the 2 configs going to be that big? Right now, only complaint is guava and I am only seeing this once in the examples given. If everything else will be the same between the 2, then it puts more work on us to keep them in synch. So for our current use, I would rank 1/3 as higher.

I do have one additional option to present that I didn't think of before. We'll call this option 4.
This whole problem is stemming from the fact we only allow one import control check in one config. We allow multiple instances of the same check for others with different properties like NoWhitespaceBefore.
We could take the files/path attribute and move it to the check and separate test and production that way. This is similar to option 2, but would allow the files to be completely separated.
Example:

    <module name="ImportControl">
      <property name="file" value="import-control-test.xml"/>
      <property name="scanPath" value="^.*[\\/]test[\\/].*$"/>
    </module>
    <module name="ImportControl">
      <property name="file" value="import-control-main.xml"/>
      <property name="scanPath" value="^.*[\\/]main[\\/].*$"/>
    </module>

Terms of ranking, I still group this with 2 and but see it as slightly higher than 2.

Final results: 1/3, 4, 2.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 22, 2016

Member

2 looks misleading.

sorry , main attributes were missed that diff main vs test. I updated my comment.

I know you like separating test from production, but is the differences between the 2 configs going to be that big?

there should no limitations for guava and any other imports usage. There should be only disallow tags for most hated imports. All other is allowed (no long list of allowed tag at all).

Member

romani commented Dec 22, 2016

2 looks misleading.

sorry , main attributes were missed that diff main vs test. I updated my comment.

I know you like separating test from production, but is the differences between the 2 configs going to be that big?

there should no limitations for guava and any other imports usage. There should be only disallow tags for most hated imports. All other is allowed (no long list of allowed tag at all).

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 22, 2016

Contributor

2 looks misleading. It does not define anywhere which one is test and which one is production except in a comment.

sorry , main attributes were missed that diff main vs test. I updated my comment.

Yes I forgot the attributes. I updated my Gist as well.

My personal preference: option 1, then option 2 and lastly option 3.

Contributor

jochenvdv commented Dec 22, 2016

2 looks misleading. It does not define anywhere which one is test and which one is production except in a comment.

sorry , main attributes were missed that diff main vs test. I updated my comment.

Yes I forgot the attributes. I updated my Gist as well.

My personal preference: option 1, then option 2 and lastly option 3.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 22, 2016

Member

I like approach 4 too, even better than 2 (as it demand less changes to xm format).

Member

romani commented Dec 22, 2016

I like approach 4 too, even better than 2 (as it demand less changes to xm format).

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 24, 2016

Member

@MEZk , please rank your preferences for proposed solutions.
Current state of voting results make no leader.

Member

romani commented Dec 24, 2016

@MEZk , please rank your preferences for proposed solutions.
Current state of voting results make no leader.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 25, 2016

Contributor

@romani @rnveach @jochenvdv

If I were to choose I would opt for the 4th option now as it completely separates test config and production. For example, I always do such thing with Spring Boot and Spring configuration itself at work, and it works pretty well. All test configs are located in test-resources folder and it is clear enough for team members what are they for. On the other hand, programmers tends to be lazy 😋 and they will not keep both configs in sync. It will become a routine to update both configurations on and on and finally test configuration will be deleted by the programmers who use the check. The 4th option also does not require to change DDT schema and this is 👍

1st option also looks confusing to me. I did not understand the idea of the option when I had read the config at first. I don't want to examine the documentation and try to figure out what is files section for. Checkstyle is just a tool and we should make it as easy to configure as it is possible. Otherwise user will just switch it off.

@rnveach
2 looks misleading. It does not define anywhere which one is test and which one is production except in a comment.

Agree now.

The 3rd option looks good, but ...

@romani
Mixing rules for production code and tests in the same file(line located close to each other) is not good and error prone.

Lets back to the main idea of the issue:

In scope of this issue we need to split configuration into two and make rules for main code more strict
This will allow us to restrict usage of Guava library (#3433) in main code and keep freedom to use convenient methods in test area.

Thus, the point is to separate two areas of the validation: test and main. The configuration of the check for the main area should be more strict than for the test area. Lets keep the main config in separate file and do not mix it with test config.

My final decision is 4, 3, 2, 1.

Contributor

MEZk commented Dec 25, 2016

@romani @rnveach @jochenvdv

If I were to choose I would opt for the 4th option now as it completely separates test config and production. For example, I always do such thing with Spring Boot and Spring configuration itself at work, and it works pretty well. All test configs are located in test-resources folder and it is clear enough for team members what are they for. On the other hand, programmers tends to be lazy 😋 and they will not keep both configs in sync. It will become a routine to update both configurations on and on and finally test configuration will be deleted by the programmers who use the check. The 4th option also does not require to change DDT schema and this is 👍

1st option also looks confusing to me. I did not understand the idea of the option when I had read the config at first. I don't want to examine the documentation and try to figure out what is files section for. Checkstyle is just a tool and we should make it as easy to configure as it is possible. Otherwise user will just switch it off.

@rnveach
2 looks misleading. It does not define anywhere which one is test and which one is production except in a comment.

Agree now.

The 3rd option looks good, but ...

@romani
Mixing rules for production code and tests in the same file(line located close to each other) is not good and error prone.

Lets back to the main idea of the issue:

In scope of this issue we need to split configuration into two and make rules for main code more strict
This will allow us to restrict usage of Guava library (#3433) in main code and keep freedom to use convenient methods in test area.

Thus, the point is to separate two areas of the validation: test and main. The configuration of the check for the main area should be more strict than for the test area. Lets keep the main config in separate file and do not mix it with test config.

My final decision is 4, 3, 2, 1.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Dec 25, 2016

Contributor

I like 4 as well. It seems like it will be the winning option?

Contributor

jochenvdv commented Dec 25, 2016

I like 4 as well. It seems like it will be the winning option?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 25, 2016

Member

Fun vote results. 4 is winner but author of it put it to third option :).
@rnveach, please share last post/ideas before we go with option 4.

Member

romani commented Dec 25, 2016

Fun vote results. 4 is winner but author of it put it to third option :).
@rnveach, please share last post/ideas before we go with option 4.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 4, 2017

Member

@romani No new ideas or anything else. Let's go with Option 4.

Member

rnveach commented Jan 4, 2017

@romani No new ideas or anything else. Let's go with Option 4.

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Jan 4, 2017

Contributor

Ok.

What should the property name be? I don't like scanPath like in @rnveach's example.
I suggest to simply use path:

<module name="ImportControl">
    <property name="file" value="import-control-test.xml"/>
    <property name="path" value="^.*[\\/]test[\\/].*$"/>
</module>
<module name="ImportControl">
    <property name="file" value="import-control-main.xml"/>
    <property name="path" value="^.*[\\/]main[\\/].*$"/>
</module>

The documentation must of course explicitly state that it is regex only.

Contributor

jochenvdv commented Jan 4, 2017

Ok.

What should the property name be? I don't like scanPath like in @rnveach's example.
I suggest to simply use path:

<module name="ImportControl">
    <property name="file" value="import-control-test.xml"/>
    <property name="path" value="^.*[\\/]test[\\/].*$"/>
</module>
<module name="ImportControl">
    <property name="file" value="import-control-main.xml"/>
    <property name="path" value="^.*[\\/]main[\\/].*$"/>
</module>

The documentation must of course explicitly state that it is regex only.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 5, 2017

Member

Please use path sometime we add suffix Pattern to let user skip documentation reading and understand type by name

Member

romani commented Jan 5, 2017

Please use path sometime we add suffix Pattern to let user skip documentation reading and understand type by name

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Jan 11, 2017

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Jan 11, 2017

Contributor

I am implementing this. I added unit tests to ImportControlCheckTest which now raises a MethodCount violation (more than 35 methods).

What should I do about this? Should I create a new test containing only the tests for the path property (this seems overkill and would require the getPath helper method to be duplicated)?

Maybe tests should be excluded from the MethodCount check?

Contributor

jochenvdv commented Jan 11, 2017

I am implementing this. I added unit tests to ImportControlCheckTest which now raises a MethodCount violation (more than 35 methods).

What should I do about this? Should I create a new test containing only the tests for the path property (this seems overkill and would require the getPath helper method to be duplicated)?

Maybe tests should be excluded from the MethodCount check?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jan 11, 2017

Contributor

@romani
MethodCount should not rise violations on test classes. Lets turn it off for UTs and ITs.

Contributor

MEZk commented Jan 11, 2017

@romani
MethodCount should not rise violations on test classes. Lets turn it off for UTs and ITs.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani
Member

romani commented Jan 11, 2017

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Jan 11, 2017

Contributor

@romani Do you mean to suppress only this test or all tests? If all tests I will make a separate PR. Right now some specific tests are already suppressed for MethodCount, but I think it makes sense to suppress the whole UT/IT directories.

Contributor

jochenvdv commented Jan 11, 2017

@romani Do you mean to suppress only this test or all tests? If all tests I will make a separate PR. Right now some specific tests are already suppressed for MethodCount, but I think it makes sense to suppress the whole UT/IT directories.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 11, 2017

Member

please do suppression for all tests in separate PR

Member

romani commented Jan 11, 2017

please do suppression for all tests in separate PR

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Jan 12, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Jan 13, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Jan 13, 2017

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Jan 15, 2017

@romani romani changed the title from ImportControl should have parameter which allows to validate specified file path to ImportControl should have property which allows to validate specified file path Jan 22, 2017

@romani romani added the new feature label Jan 22, 2017

@romani romani added this to the 7.5 milestone Jan 22, 2017

@romani romani closed this Jan 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment