Skip to content
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

Feature: "exclude directories from analysis" #44

Conversation

KayWeinert
Copy link
Contributor

@KayWeinert KayWeinert commented Mar 16, 2021

Summary

A solution for the feature request to exclude generated sources from being analysed.

Details

With this feature arch-unit-maven-plugin is able to exclude java files in any source folder from being analysed by arch-unit.
You only have to define the folders within the arch-unit-maven-plugin configuration. It can be an absolute or a relative path.
All java files in this folders and sub folders will be ignored by arch-unit and of course it works in multi module maven projects.

This solution requires a merge of the pull request: societe-generale/arch-unit-build-plugin-core#47

<plugin>
            <groupId>com.societegenerale.commons</groupId>
            <artifactId>arch-unit-maven-plugin</artifactId>
             <version>2.7.3-SNAPSHOT</version>
            <configuration>                
                <excludedPaths>
                    <excludedPath>generated-sources</excludedPath>
                    <excludedPath>annotations</excludedPath>
                </excludedPaths>

                <rules> ... </rules>

</plugin>

Checklist:

  • I've added tests for my code.
  • Documentation has been updated accordingly to this PR

Related issue :

@KayWeinert KayWeinert marked this pull request as ready for review March 16, 2021 23:44
@KayWeinert KayWeinert force-pushed the feature/exclude_generated_sources branch from c29cafb to afc137f Compare April 12, 2021 19:26
@KayWeinert
Copy link
Contributor Author

pom upgrade to 2.7.4-SNAPSHOT

@KayWeinert
Copy link
Contributor Author

My PR is now open for quite a while ... are there any questions or problems?

@vincent-fuchs
Copy link
Contributor

sorry, I had totally missed it, thanks for bringing that up.

I just had a look : apart from the changes in ArchUnitMojo , the other changes are not Maven specific, right ? so I think we should put them in https://github.com/societe-generale/arch-unit-build-plugin-core , so that JavaFileParser and ExcludedPathsPreProcessor can be reused also in the gradle plugin.

what do you think ? I could try to do it in the next couple of days

@KayWeinert
Copy link
Contributor Author

I just had a look : apart from the changes in ArchUnitMojo , the other changes are not Maven specific, right ? so I think we should put them in https://github.com/societe-generale/arch-unit-build-plugin-core , so that JavaFileParser and ExcludedPathsPreProcessor can be reused also in the gradle plugin.

what do you think ? I could try to do it in the next couple of days

Right the other changes are not Maven specific, so let's try it.

@vincent-fuchs
Copy link
Contributor

vincent-fuchs commented Jul 10, 2021

@vincent-fuchs
Copy link
Contributor

hi @KayWeinert , I had a look at the code, but I have a doubt : what does your change do that the existing behavior doesn't ? we are already able to exclude a single class or a package, or a directory :

https://github.com/societe-generale/arch-unit-build-plugin-core/blob/exclude_generated_source_copyFromMavenRepo/src/main/java/com/societegenerale/commons/plugin/utils/ExclusionImportOption.java#L33-L46

can you confirm a use case that your add-on addresses, that isn't covered today ?

thanks

@KayWeinert
Copy link
Contributor Author

KayWeinert commented Jul 14, 2021

Hi @vincent-fuchs,

if my configuration is correct than there is in my opinion no way to exclude all java files within a directory like generated-sources. You can do this only by defining a special package or a class name prefix for generated sources but that not what I want.

I want to have my generated files to have the same package name (package visibility) and same naming pattern such as my self written files but only my own files should be analysed by arch-unit rules. So that is the use case of my PR.

If there is already a way to do realise this use case please, let me know it.

This is my arch-unit-maven-plugin configuration for a multi module maven project wich only works with if the changes of the PR a present. :

<plugin>
	<groupId>com.societegenerale.commons</groupId>
	<artifactId>arch-unit-maven-plugin</artifactId>	
	<version>2.7.3</version>
	<configuration>		
		<excludedPaths>
			<excludedPath>generated-sources</excludedPath>
			<excludedPath>annotations</excludedPath>
		</excludedPaths>

		<rules>
			<!-- using a rule available out of the box... -->
			<preConfiguredRules>
				<rule>com.societegenerale.commons.plugin.rules.NoPublicFieldRuleTest</rule>
				<rule>com.societegenerale.commons.plugin.rules.NoStandardStreamRuleTest</rule>
				<rule>com.societegenerale.commons.plugin.rules.NoJodaTimeRuleTest</rule>
				<rule>com.societegenerale.commons.plugin.rules.NoPrefixForInterfacesRuleTest</rule>
			</preConfiguredRules>
		</rules>
	</configuration>
	<executions>
		<execution>
			<phase>test</phase>
			<goals>
				<goal>arch-test</goal>
			</goals>
		</execution>
	</executions>
 </plugin>

@vincent-fuchs
Copy link
Contributor

Have you been able to identify why the code we put last time in https://github.com/societe-generale/arch-unit-build-plugin-core/blob/master/src/main/java/com/societegenerale/commons/plugin/utils/ExclusionImportOption.java is not able to exclude a given directory ?

I guess my test is not realistic enough, but here's a gist with a test trying to show that configuring "generated-sources" as an excludedPath will do what is expected. But I guess in reality, something is different and it doesn't work : do you know why ?

could you add a test in https://github.com/societe-generale/arch-unit-build-plugin-core/blob/master/src/test/java/com/societegenerale/commons/plugin/utils/ExclusionImportOptionTest.java , building proper Location objects as inputs to show that current ExclusionImportOption is not handling this case ?

I am asking because with this PR (or the one we would do in https://github.com/societe-generale/arch-unit-build-plugin-core ), we would add quite a lot of code (that I would then have to maintain), for a use case that seems very close to an existing one : so I'd like to be sure to understand why we need so much code, and an "simple" enhancement on ExclusionImportOption would not be enough.

@KayWeinert
Copy link
Contributor Author

ExclusionImportOption.java is working fine if you are looking only to compiled code.

"generated-sources" is not a folder of target/classes and so it can not be excluded. arch-unit-build-plugin-core is only working on compiled classes and packages and that is the problem. After compiling generated and self written code is mixed together in the same folders and packages.

The solotion is to analyse all java files (not class files) within the exclude-folders by arch-unit-maven-plugin before starting the arch-unit tests via arch-unit-build-plugin-core. After the analysis arch-unit-maven-plugin knows all full qualified class names to be excluded. By using this information additional to "generated-sources" arch-unit-build-plugin-core is able exclude the right class files.

Because of the combination of arch-unit-build-plugin-core and arch-unit-maven-plugin I can not write a unit test just to ExclusionImportOptionTest.java.

My PR contains the test "ArchUnitMojoExcludeTest" which considers the combination.

By changing ArchUnitMojo#execute() from this

...
final Set<String> excludes = new ExcludedPathsPreProcessor().processExcludedPaths(getLog(), projectBuildDir, excludedPaths);
            ruleInvokerService = new RuleInvokerService(mavenLogAdapter, new MavenScopePathProvider(mavenProject), excludes);

...

to that


...
ruleInvokerService = new RuleInvokerService(mavenLogAdapter, new MavenScopePathProvider(mavenProject), excludedPaths);
...


everything will work as without my changes and ArchUnitMojoExcludeTest will fail.

Hopefully I could make my self clear.

@vincent-fuchs
Copy link
Contributor

Thanks a lot, now I get it ;-)

I think I am mostly done in societe-generale/arch-unit-build-plugin-core#49 , which I think is ready to merge..

Then the code in the Maven specific plugin would be simplified : #45 . It fails for now because it references a -SNAPSHOT version, but if you build both locally it should work.

Could you review it in case I missed anything ?

Thanks

@KayWeinert
Copy link
Contributor Author

I've reviewed your changes in societe-generale/arch-unit-build-plugin-core#49 and #45. Looks good to me. I've tested both together within a real project and it works as expected.

@daniel-shuy
Copy link

@KayWeinert @vincent-fuchs looking forward to this feature, thanks so much for this!

@vincent-fuchs
Copy link
Contributor

was implemented in #45

@vincent-fuchs
Copy link
Contributor

v2.8.0 has been released - please give it a try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants