Issue #13: First approach

[Macarse]

This is just a first approach to issue #13. @stephanenicolas and I were discussing how to this for some time so I gave it a shot. It's far from finish but I would like to have some feedback.
Instead of a factory I used the information gather by dart to create a separate class which works as an intent builder.

In our app we are trying to get rid of the IntentFactory pattern and let every activity be responsible of it's API via a builder. We hope we can make Dart take care of that.

Some open questions:

• It doesn't make a lot of sense of me that ExtraInjection has a Set<FieldBinding> instead of a single one. Why is that?
• I didn't handle the parcel case. I am not 100% sure how it should be done
• @f2prateek, does it make sense to you for IntentBuilder to get information from ExtraInjector? I am not very happy with the getters added
• I haven't found a good way to handle mandatory / optional parameters. Any recommendation?

[f2prateek]

Thanks, I'll try to take a look this week. For the first question, this test case illustrates why multiple field bindings are required. Basically the same key could have multiple field bindings.

[stephanenicolas]

PackageName

[Macarse]

it comes from the variable name in ExtraInjector. I didn't want to change the existing code.

[stephanenicolas]

The biggest issue with this PR is that it should use a templating system to generate code.
Also a mandatory field check would be very nice for 1.0 release of the annotation processor.

And tests are missing to see if the annotation processor generates the right code. You can use "compile testing" or truth from Google for that.

[stephanenicolas]

FQCN

[Macarse]

comes from existing code

[johncarl81]

Pleased to see this getting some attention. I'll try to put some time into a review as well.

[f2prateek]

Thanks @stephanenicolas and @johncarl81!

[stephanenicolas]

I am reviewing the PR again. Here is the TODO list : (after talking with @Macarse, héhé, we work together)

• Adding unit tests to check generated classes.
• check if we limit the injectextra/intent builder to activities (nope there is no such limitation in Dart)
• fix the parcel / check serializable
• check multiple bindings for the same key, how do we handle that ?
• add tests to see if the intent is generated properly and inject extra can get the params. We think dart doesn't have those tests itself.

[f2prateek]

@stephanenicolas The test for the last is in the sample module - https://github.com/f2prateek/dart/blob/master/dart-sample/src/test/java/com/f2prateek/dart/example/SampleActivityTest.java

[stephanenicolas]

Thx @f2prateek . I am gonna work on that today.
Btw, there is another question, and I don't want to influence the answer : but are you 100 % sure you want to add this feature to Dart. I understand dart is very sharply focused, as butterknife is for instance. And adding such a new feature to Dart might be a little change of its actual focus.
The only alternative I see would be to create :

• a shared artifact for Dart annotations
• a shared artifact for the common part of annotation processor common to Dart and this new intent builder feature.
  But it looks a bit heavy, and maybe after all that should simply fit naturally into Dart.

WDYT ?

[f2prateek]

I think it's useful to have this feature, but I agree there should be a way to seperate the binding features from the factory features.

I had considered two ways to do this.

1. The solution you proposed - a seperate annotation processor for generating the factories (and we use common code through a shared module).
2. Add an extra annotation (@GenerateFactory or something similar) for use on activities (and maybe fragments). This way the factories only get generated if the user has explicitly requested them, but simplifies our implementation.

I'm in favour of the latter, seems like there is much less ceremony around it.

[stephanenicolas]

Hi @f2prateek , thanks for your kind answer. From a user / dev perspective, I think solution 2 is really error prone : you have to annotate a class to get a behaviour..
It would prefer it the other way around. I mean, if you are to adopt this solution, why not having an annotation that prevents build from being generated ?

But if you really consider this feature is not part of the core of Dart (and to my mind this can be defended, I would say it is just a different aspect of Dart), we should split the library. I mean either split in a different library project with its own repo or as a new module in dart. But the best to enable/disable the feature would be to add or not a dependency to your build, as a dev.

But I think we might actually be over designing, to me it's quite possible that this is just a core Dart feature, just another aspect of extra injection.

[stephanenicolas]

sorry for the multi builds, I was blind, There was a problem in my pom files about the plugin (android maven plugin being differnt in parent and pom, not taking into account the groupId change).

Next commit is good.

[stephanenicolas]

@f2prateek This commit should be green. Everything works locally. So we :

• Added unit tests to check generated classes of the intent builder
• made it very clean and simple to use the parceler lib with the intent builder and Dart. (you never have to wrap or unwrap basically).
• support multiple bindings for the same key, only one method is generated in the intent builder
• added a test to see if the intent is generated properly and inject extra can get the params. This is part of the sample app tests.
• updated the sample app.

[Macarse]

this shouldn't be hardcoded here.

[Macarse]

@johncarl81 @f2prateek: Can you guys review?

[f2prateek]

Is this absolutely required? Since Dart doesn't have seperate artifacts for the annotation processing and runtime API, we don't want to add these dependencies to the the user's app.

[johncarl81]

Hmm, are there any class required at runtime in Dart? Could you just make all of Dart provided?

[stephanenicolas]

@johncarl81 , yes Dart as some class used at runtime. Namely, users must call Dart.inject(context) to invoke that has been generated during annotation processing.

@f2prateek , the intent builder's code generation is much simpler with this dependency.
We could remove it for sure, but we would loose quality and maintenance comfort.
We can try the provided scope, it should work according to http://docs.codehaus.org/display/MAVENUSER/Dependency+Scopes

But maybe a better solution would be to separate runtime and compiler artifacts. Btw, @f2prateek , I thought you relied on proguard to remove the Dart annotation processor from final apk. And the same reasoning would have applied to any dependency here. But how does Dart annotation processor avoid getting included in final apk and not this dependency, so ?

[f2prateek]

I don't use proguard, which is why the goal was to make the code generation part free of dependencies.

I'm not entirely sure provided would work here, since the annotation processing happens when a user includes the dart jar in their classpath. By marking javapoet as provided, javapoet won't be available on the classpath when the user is compiling their app.

I'm also working on splitting the artifacts for v2

[stephanenicolas]

Can we agree on either first splitting the artifacts or accepting the PR
with JavaPoet as a dependency ?
@f2prateek ?

2015-05-16 12:18 GMT-07:00 Prateek Srivastava notifications@github.com:

In dart/pom.xml
#33 (comment):

@@ -35,6 +35,18 @@
compile-testing
test

•  <groupId>org.parceler</groupId>
  

•  <artifactId>parceler-api</artifactId>
  

•  <scope>test</scope>
  

•    <groupId>com.squareup</groupId>
  

•    <artifactId>javapoet</artifactId>
  

I don't use proguard, which is why the goal was to make the code
generation part free of dependencies.

I'm not entirely sure provided would work here, since the annotation
processing happens when a user includes the dart jar in their classpath. By
marking javapoet as provided, javapoet won't be available on the classpath
when the user is compiling their app.

I'm also working on splitting the artifacts for v2

—
Reply to this email directly or view it on GitHub
https://github.com/f2prateek/dart/pull/33/files#r30463728.

[johncarl81]

+1 for splitting apj/processor to avoid unnecessary runtime inclusions.

[f2prateek]

Splitting the artifacts will be part of a v2 release. I'll clean this up and drop and JavaPoet dependency for now.

[stephanenicolas]

I will follow up on this at the end of the PR conversation.

[johncarl81]

In Transfuse I opted for mandatory extras to be handled as part of the first builder phase (in my case the constructor) then optional extras as optional methods in phase two. My idea of how this should work in Dart is like this:

new SampleActivityIntentBuilder(context, requiredExtra1, requiredExtra2...)
    .setOptionalExtra3(extra3)
    .setOptionalExtra4(extra4);

This makes it much less likely that the end user will miss a mandatory extra

I like the null check on the extras druing the build(), in the case above these could be handled a little earlier in the constructor.

[johncarl81]

Although these may not compile successfully, why not store them as actual Java files? We could put them in a separate non-source path.

[stephanenicolas]

we just applied the recipe of current dart code. But yeah, a file would sound great, but not in this PR.

[stephanenicolas]

@johncarl81 about the constructor pattern :
We have also been thinking of a DSL that would be even more convenient, compared to a builder pattern. But it is a bit more complicated that our current model.
And personally, I prefer our current methods with meaningful names for each param more than a constructor's parameter. But that is really a matter of taste I guess. Though your way provides more constrain check at compile time...

We would have to all agree on the API before we change something like that.

[johncarl81]

👍

[stephanenicolas]

Yeah, this is great feature.

[johncarl81]

@stephanenicolas, Yes, I prefer compile time errors over runtime errors where possible. You could use Javadocs and parameter names to help with knowing what parameters are which.

[stephanenicolas]

@johncarl81 me too I must say, but still, I don't like to have to pass all params in one method call.
So maybe the only right way to do it and get the both of both worlds (compile errors and a nice syntax) would be a DSL.
(there is a pretty neat way to do it using enumerations that I never tried and would like to, even though it will be the hard way as it's not only implementing it but creating a generator to implement it..)

If we target that for a next release then our current implementations is probably a good basis for a first shot.

[stephanenicolas]

Here is more or less what could the DSL generator look like. Indeed, it may not be so hard to implement :

//the real intent builder would actually 
//define a constructor with a context
//intent is a fake class below
public class IntentBuilder {
    private Intent intent = new Intent();

    //starting point
    public AfterSettingA withA(A a) {
        intent.putExtra(a);
        return new AfterSettingA() {};
    }

    //intermediary states for mandatory fields
    //one per field
    public class AfterSettingA {
        public AllSet withB(B b) {
            intent.putExtra(b);
            return new AllSet() {};
        }
    }

    //final state, also allows to use optional fields.
    public class AllSet {
        //one method per optional field, same return type
        public AllSet withC(C c) {
            intent.putExtra(c);
            return this;
        }

       //final method
        public Intent build() {
            return intent;
        }
    }
}

[johncarl81]

@stephanenicolas, I have to say I like this approach. This has the benefit of clearly asking for each parameter by name while leveraging compile time to enforce required fields.

One nit, I'd make sure to do a null check in each required DSL setter:

    //starting point
    public AfterSettingA withA(A a) {
        if(a == null){
            throw new IllegalStateException("Parameter a is mandatory");
        }
        intent.putExtra(a);
        return new AfterSettingA() {};
    }

[stephanenicolas]

@f2prateek : Follow up on the v2 idea and removing javapoet.

Ok. Good for me. I think it is for @Macarse as well. So, you take it, right ?

For the V2, chances are that we contribute. The best would be to have

• a parent module
• a common classes processor module
• dart annotation processor module
• intent builder annotation processor module
• the runtime classes module
• the annotation classes module

Actually, in a v2, would you mind to rename the intent builder to 'Henson' ? I had this idea in mind a long time ago of creating an annotation processor to generate a navigation sub-layer between activities : https://github.com/stephanenicolas/henson. I never had time to code it, it was intended to work with the byte code weaving InjectExtra equivalent of Dart I created last year, but I am happy to participate to coding it in Dart. However I find the name is nice and for good reasons. Do you mind ?

[stephanenicolas]

@johncarl81 we should double check with current optional / nullable contract of Dart for that. I didn't follow the latest changes. Wow, guys, we got a lot of features for a V2 here, but it really seems a lot for a PR !

Should we create a new V2 milestone, create a branch, and starting issuing tickets and coding ? Btw, I would be glad to join the contributors' team if that's possible.

[Macarse]

status?

[f2prateek]

@Macarse I'm working on removing the JavaPoet dependency.

[stephanenicolas]

Hi @f2prateek , do you have any progress ? We are really looking forward to this feature.
Can we help ?

[stephanenicolas]

https://github.com/emilsjolander/IntentBuilder