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: allow regex in subpackage elements. #2999

Closed
effjay opened this Issue Mar 3, 2016 · 18 comments

Comments

Projects
None yet
5 participants
@effjay

effjay commented Mar 3, 2016

Hi all,

at the moment it is not possible to use the same import control configuration for multiple subpackages.

What I need to do is this:

<subpackage pkg="de.main">
   <subpackage pkg="*"> 
      <subpackage pkg="service">
         <allow...> 
      </subpackage>
   </subpackage>
</subpackage>

This way, the package control configuration for the service subpackage would match all modules at once, like de.main.mod1.service as well as de.main.mod2.service and so on. If someone creates a mod3 module, then the configuration would still work.

I couldn't get it to work, as wildcards and regex are only valid on allow or disallow elements right now.

For this, a new attribute might need to be created for the "subpackage" element: "regex"="true/false". That is required to be consistent with the design of the "allow" element.

This will mean that http://www.puppycrawl.com/dtds/import_control_1_1.dtd must be changed to version 1.2 as well.

Thanks for any help on this, best regards
Daniel

@romani

This comment has been minimized.

Show comment
Hide comment

@romani romani added the approved label Mar 3, 2016

@romani romani changed the title from Feature: allow regex in subpackage elements. to ImportControl: allow regex in subpackage elements. Mar 3, 2016

@DyosTheNerd

This comment has been minimized.

Show comment
Hide comment
@DyosTheNerd

DyosTheNerd Jul 5, 2016

As I also would like to see this feature for my next project, but never really used github, how can I start contributing?

As I also would like to see this feature for my next project, but never really used github, how can I start contributing?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 5, 2016

Member

@DyosTheNerd
Check out the following links:
http://checkstyle.sourceforge.net/contributing.html
http://checkstyle.sourceforge.net/beginning_development.html (has more specific details of the 2 for starting)

Besides knowing Java, you will have to be able to work with Maven and Git.
Questions may be asked on Gitter or Google Groups.

Member

rnveach commented Jul 5, 2016

@DyosTheNerd
Check out the following links:
http://checkstyle.sourceforge.net/contributing.html
http://checkstyle.sourceforge.net/beginning_development.html (has more specific details of the 2 for starting)

Besides knowing Java, you will have to be able to work with Maven and Git.
Questions may be asked on Gitter or Google Groups.

@romani

This comment has been minimized.

Show comment
Hide comment
Member

romani commented Jul 5, 2016

@DyosTheNerd , please review https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki it was written for beginners.

@vboerchers

This comment has been minimized.

Show comment
Hide comment
@vboerchers

vboerchers Sep 7, 2016

@DyosTheNerd

What is your status on this issue? I have a need for this feature too and I am considering working on it.

@DyosTheNerd

What is your status on this issue? I have a need for this feature too and I am considering working on it.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 7, 2016

Member

@vboerchers , nothing was done for 2 month it usually mean that he has no time to do this.
please be welcome to provide a patch.

Member

romani commented Sep 7, 2016

@vboerchers , nothing was done for 2 month it usually mean that he has no time to do this.
please be welcome to provide a patch.

@vboerchers

This comment has been minimized.

Show comment
Hide comment
@vboerchers

vboerchers Sep 9, 2016

I have a question regarding XML design: I have started by
<subpackage pkg=".*" regex="true"> since this is the way it's implemented for <allow> and <disallow> but I'm wondering if

<subpackage regex=".*">

wouldn't be nicer. What do you think?

[edit: I found a statement in the googlegroups discussion that the syntax should be consistent with the allow/disallow syntax. So I stick to that]

vboerchers commented Sep 9, 2016

I have a question regarding XML design: I have started by
<subpackage pkg=".*" regex="true"> since this is the way it's implemented for <allow> and <disallow> but I'm wondering if

<subpackage regex=".*">

wouldn't be nicer. What do you think?

[edit: I found a statement in the googlegroups discussion that the syntax should be consistent with the allow/disallow syntax. So I stick to that]

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 11, 2016

@vboerchers

This comment has been minimized.

Show comment
Hide comment
@vboerchers

vboerchers Sep 13, 2016

@romani, I have created a pull request #3438 that unfortunately cannot pass the travis build before the new DTD was uploaded. So the new DTD had to be reviewed first. Please note that I had to change the type of the package specification (name/pkg) from NMTOKEN to CDATA.

@romani, I have created a pull request #3438 that unfortunately cannot pass the travis build before the new DTD was uploaded. So the new DTD had to be reviewed first. Please note that I had to change the type of the package specification (name/pkg) from NMTOKEN to CDATA.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 13, 2016

Member

@vboerchers , thanks a lot, please give me some time to review , looks like to need to move all dtds to sourceforge first , as I do not have access to puppycrawl domain. See #1571.

<subpackage regex=".*"> wouldn't be nicer. What do you think?

NO, we should be consistent with existing names. pkg should be used.

Member

romani commented Sep 13, 2016

@vboerchers , thanks a lot, please give me some time to review , looks like to need to move all dtds to sourceforge first , as I do not have access to puppycrawl domain. See #1571.

<subpackage regex=".*"> wouldn't be nicer. What do you think?

NO, we should be consistent with existing names. pkg should be used.

@vboerchers

This comment has been minimized.

Show comment
Hide comment
@vboerchers

vboerchers Sep 13, 2016

@romani what about making the 1.2 DTD the first one `located on sf.net? Then I would change my PR accordingly to make the build pass and you could do #1571 later.

@romani what about making the 1.2 DTD the first one `located on sf.net? Then I would change my PR accordingly to make the build pass and you could do #1571 later.

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 14, 2016

@vboerchers

This comment has been minimized.

Show comment
Hide comment
@vboerchers

vboerchers Sep 14, 2016

@romani I have updated pull request #3438

@romani I have updated pull request #3438

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 14, 2016

Member

Change from NMTOKEN to CDATA is ok.
NMTOKEN values - https://www.w3.org/TR/2000/WD-xml-2e-20000814#sec-common-syn do have characters that package name does not allow. So I do not see reason to stay on it.

your schema file was changed a bit and uploaded:
http://checkstyle.sourceforge.net/dtds/import_control_1_2.dtd

Member

romani commented Sep 14, 2016

Change from NMTOKEN to CDATA is ok.
NMTOKEN values - https://www.w3.org/TR/2000/WD-xml-2e-20000814#sec-common-syn do have characters that package name does not allow. So I do not see reason to stay on it.

your schema file was changed a bit and uploaded:
http://checkstyle.sourceforge.net/dtds/import_control_1_2.dtd

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 14, 2016

Member

In checkstyle we use checkstyle over its own code. We force ourself to eat food we produce.
Can you propose update for https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml ? to start to use new feature.

Member

romani commented Sep 14, 2016

In checkstyle we use checkstyle over its own code. We force ourself to eat food we produce.
Can you propose update for https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml ? to start to use new feature.

@vboerchers

This comment has been minimized.

Show comment
Hide comment
@vboerchers

vboerchers Sep 14, 2016

Can you propose update for https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml ? to start to use new feature.

Given the rules in this file I don't see a good use case for package regexps. Even allow/disallow rules are not used in it. So I propose to just update the DTD header to use version 1.2.

Can you propose update for https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml ? to start to use new feature.

Given the rules in this file I don't see a good use case for package regexps. Even allow/disallow rules are not used in it. So I propose to just update the DTD header to use version 1.2.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 14, 2016

Member

Yes, do at least this. I will try to think about some rule.

Member

romani commented Sep 14, 2016

Yes, do at least this. I will try to think about some rule.

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 14, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 14, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 14, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 14, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 17, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 17, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 17, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 20, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 21, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 21, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 21, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 24, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 27, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 29, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 29, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 29, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Sep 30, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 2, 2016

Member

Given the rules in this file I don't see a good use case for package regexps.

I failed to find good example for our code. So we need to make special focus on example in documentation.

Even allow/disallow rules are not used in it.

we started to use that for allow/disallow recently.

So I propose to just update the DTD header to use version 1.2.

yes, that is fine.

Member

romani commented Oct 2, 2016

Given the rules in this file I don't see a good use case for package regexps.

I failed to find good example for our code. So we need to make special focus on example in documentation.

Even allow/disallow rules are not used in it.

we started to use that for allow/disallow recently.

So I propose to just update the DTD header to use version 1.2.

yes, that is fine.

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 8, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 9, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 9, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 9, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 9, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 11, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 12, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 15, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 16, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 16, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 16, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 16, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 16, 2016

vboerchers added a commit to vboerchers/checkstyle that referenced this issue Oct 17, 2016

romani added a commit that referenced this issue Oct 18, 2016

@romani romani added the new feature label Oct 18, 2016

@romani romani added this to the 7.2 milestone Oct 18, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 18, 2016

Member

fix is merged.

Member

romani commented Oct 18, 2016

fix is merged.

@romani romani closed this Oct 18, 2016

@vboerchers

This comment has been minimized.

Show comment
Hide comment
@vboerchers

vboerchers Oct 19, 2016

Thank you, Roman!

Thank you, Roman!

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