Fix a logic to find common package name #147

sakuna63 · 2017-01-09T12:59:45Z

Henson failed to find common package name when we have some classes annotated @InjectExtra in different packages.

e.g.

If we have following classes

com.example.activity.MyActivity
com.example.fragment.MyFragment

henson-processor find com.example.. as common package name and outputs following error

javax.annotation.processing.FilerException: Illegal name com.example..Henson
        at com.sun.tools.javac.processing.JavacFiler.checkName(JavacFiler.java:495)
        at com.sun.tools.javac.processing.JavacFiler.checkNameAndExistence(JavacFiler.java:517)
        at com.sun.tools.javac.processing.JavacFiler.createSourceOrClassFile(JavacFiler.java:396)
        at com.sun.tools.javac.processing.JavacFiler.createSourceFile(JavacFiler.java:378)
        at com.f2prateek.dart.henson.processor.HensonExtraProcessor.process(HensonExtraProcessor.java:138)
        at com.sun.tools.javac.processing.JavacProcessingEnvironment.callProcessor(JavacProcessingEnvironment.java:794)
        at com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:705)
        at com.sun.tools.javac.processing.JavacProcessingEnvironment.access$1800(JavacProcessingEnvironment.java:91)
        at com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1035)
        at com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1176)
        at com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1170)
        at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:856)
        at com.sun.tools.javac.main.Main.compile(Main.java:523)
        at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:129)
        at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:138)
        at org.gradle.api.internal.tasks.compile.JdkJavaCompiler.execute(JdkJavaCompiler.java:46)
        at org.gradle.api.internal.tasks.compile.JdkJavaCompiler.execute(JdkJavaCompiler.java:33)
        at org.gradle.api.internal.tasks.compile.NormalizingJavaCompiler.delegateAndHandleErrors(NormalizingJavaCompiler.java:104)
        at org.gradle.api.internal.tasks.compile.NormalizingJavaCompiler.execute(NormalizingJavaCompiler.java:53)
        at org.gradle.api.internal.tasks.compile.NormalizingJavaCompiler.execute(NormalizingJavaCompiler.java:38)
        at org.gradle.api.internal.tasks.compile.CleaningJavaCompilerSupport.execute(CleaningJavaCompilerSupport.java:35)
        at org.gradle.api.internal.tasks.compile.CleaningJavaCompilerSupport.execute(CleaningJavaCompilerSupport.java:25)
        at org.gradle.api.tasks.compile.JavaCompile.performCompilation(JavaCompile.java:189)
        at org.gradle.api.tasks.compile.JavaCompile.compile(JavaCompile.java:170)
        at org.gradle.api.tasks.compile.JavaCompile.compile(JavaCompile.java:113)
        at com.android.build.gradle.tasks.factory.AndroidJavaCompile.compile(AndroidJavaCompile.java:49)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.gradle.internal.reflect.JavaMethod.invoke(JavaMethod.java:75)
        at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$IncrementalTaskAction.doExecute(DefaultTaskClassInfoStore.java:158)
        at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$StandardTaskAction.execute(DefaultTaskClassInfoStore.java:129)
        at org.gradle.api.internal.project.taskfactory.DefaultTaskClassInfoStore$StandardTaskAction.execute(DefaultTaskClassInfoStore.java:118)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeAction(ExecuteActionsTaskExecuter.java:80)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeActions(ExecuteActionsTaskExecuter.java:61)
        at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:46)
        at org.gradle.api.internal.tasks.execution.PostExecutionAnalysisTaskExecuter.execute(PostExecutionAnalysisTaskExecuter.java:35)
        at org.gradle.api.internal.tasks.execution.SkipUpToDateTaskExecuter.execute(SkipUpToDateTaskExecuter.java:64)
        at org.gradle.api.internal.tasks.execution.ValidatingTaskExecuter.execute(ValidatingTaskExecuter.java:58)
        at org.gradle.api.internal.tasks.execution.SkipEmptySourceFilesTaskExecuter.execute(SkipEmptySourceFilesTaskExecuter.java:52)
        at org.gradle.api.internal.tasks.execution.SkipTaskWithNoActionsExecuter.execute(SkipTaskWithNoActionsExecuter.java:52)
        at org.gradle.api.internal.tasks.execution.SkipOnlyIfTaskExecuter.execute(SkipOnlyIfTaskExecuter.java:53)
        at org.gradle.api.internal.tasks.execution.ExecuteAtMostOnceTaskExecuter.execute(ExecuteAtMostOnceTaskExecuter.java:43)
        at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker.execute(DefaultTaskGraphExecuter.java:233)
        at org.gradle.execution.taskgraph.DefaultTaskGraphExecuter$EventFiringTaskWorker.execute(DefaultTaskGraphExecuter.java:215)
        at org.gradle.execution.taskgraph.AbstractTaskPlanExecutor$TaskExecutorWorker.processTask(AbstractTaskPlanExecutor.java:74)
        at org.gradle.execution.taskgraph.AbstractTaskPlanExecutor$TaskExecutorWorker.run(AbstractTaskPlanExecutor.java:55)
        at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:54)
        at org.gradle.internal.concurrent.StoppableExecutorImpl$1.run(StoppableExecutorImpl.java:40)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
        at java.lang.Thread.run(Thread.java:745)

stephanenicolas · 2017-01-09T19:40:36Z

@sakuna63 can you 1-2 tests ? Ideally a test that would break before and the would now be green with your PR.

Then we can merge it. Thx a lot !

sakuna63 · 2017-01-10T00:47:09Z

@stephanenicolas
I found a fundamental issue while I write a test.

the issue is that if we have following classes

test.test1.TestOne
test.test2.TestTwo

henson-processor find test.test as common package.

So I fix the issues and add test. Could you review new commits?

sakuna63 · 2017-01-10T00:48:43Z

Oh...CI fails 😩 I'll try to fix it.

stephanenicolas · 2017-01-10T01:09:32Z

...rocessor/src/test/java/com/f2prateek/dart/henson/processor/HensonNavigatorGeneratorTest.java

@@ -278,4 +283,110 @@
        .and()
        .generatesSources(expectedSource1);
  }
+
+  @Test public void samePackage() throws Exception {


we should add unit tests for the code you added. Not a generator test to test a lookup method in package name..

stephanenicolas · 2017-01-10T01:10:18Z

...on-processor/src/main/java/com/f2prateek/dart/henson/processor/HensonNavigatorGenerator.java

@@ -137,16 +140,36 @@ private String findCommonPackage(Collection<InjectionTarget> targets) {

  private String findCommonPackage(String commonPackageName, String packageName) {


this is the method that you should test, in a unit test.

stephanenicolas · 2017-01-10T17:22:27Z

We don't understand. can't we do something a bit simpler. For instance:

why do we have the additional dot ideally ?
it should be related to keeping the dot when finding the common part.
we could just remove it and this would solve the bug
though you raised a good bug with the case com.foo1 & com.foo2.
to solve it we could change the above algorith: the common part has to end with a dot (then we remove it before returning), otherwise we find the first previous dot and this is the new common part (and we also remove the final dot then).

This is much simpler and with unit tests it would be accepted. Are you interested ?

Rainer-Lang · 2017-09-21T15:48:35Z

@sakuna63 @stephanenicolas Any news?

dlemures · 2018-06-22T21:33:15Z

@sakuna63 can you test if the issue happens with the latest release 3.0.1?

Fix a logic to find common package name

aabfc16

sakuna63 added 2 commits January 10, 2017 09:37

Fix foundamental issue

4a49659

Add test

3f2e1bd

stephanenicolas requested changes Jan 10, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a logic to find common package name #147

Fix a logic to find common package name #147

sakuna63 commented Jan 9, 2017

stephanenicolas commented Jan 9, 2017

sakuna63 commented Jan 10, 2017 •

edited

Loading

sakuna63 commented Jan 10, 2017

stephanenicolas Jan 10, 2017

stephanenicolas Jan 10, 2017

stephanenicolas commented Jan 10, 2017

Rainer-Lang commented Sep 21, 2017

dlemures commented Jun 22, 2018 •

edited

Loading

		@@ -137,16 +140,36 @@ private String findCommonPackage(Collection<InjectionTarget> targets) {

		private String findCommonPackage(String commonPackageName, String packageName) {

Fix a logic to find common package name #147

Are you sure you want to change the base?

Fix a logic to find common package name #147

Conversation

sakuna63 commented Jan 9, 2017

stephanenicolas commented Jan 9, 2017

sakuna63 commented Jan 10, 2017 • edited Loading

sakuna63 commented Jan 10, 2017

stephanenicolas Jan 10, 2017

Choose a reason for hiding this comment

stephanenicolas Jan 10, 2017

Choose a reason for hiding this comment

stephanenicolas commented Jan 10, 2017

Rainer-Lang commented Sep 21, 2017

dlemures commented Jun 22, 2018 • edited Loading

sakuna63 commented Jan 10, 2017 •

edited

Loading

dlemures commented Jun 22, 2018 •

edited

Loading