Implement bundlers to support custom serializers

[ultimate-deej]

We discussed the issue lately in #20, and I decided to try to actually implement this.
Some comments on the implementation:

• State bundler is identical to ArgsBundler in FragmentArgs
• There is no analog of NoneArgsBundler
• Bundlers are stored in a map the same way as injectors

Also I'm very new to both Clojure and annotation processing, so I won't be surprised if you say everything is terribly wrong or something like that. However I'd appreciate any advice on how to improve it, especially considering the fact you are obviously more experienced in these.

[frankiesardo]

Wow, thanks @ultimate-deej I appreciate you taking the time for writing this.

Your approach looks generally good, here's a few pointers from my point of view:

• Most important thing is to add tests for it. There is a namespace that covers most of the edge cases and it will be good to have a nice coverage for this new features (especially for things like generics etc., they always reserve some nasty surprises). You can start by downloading leiningen and run lein test in the processor project.
• It would be very useful to add some examples for this in the sample project to show usage and edge cases
• About the implementation: I'm not too convinced about the BUNDLERS map. Because we have the name of the bundlers at compiler time I was thinking about generating a private static final FooBundler bundlerForFoo = new FooBundler() at the top of the $$Icicle helper that needs them. Something like

{{bundlers}}
private static final {{class}}  {{name}} = new {{class}} ();
{{/bundlers}}

That should make it much more performant (and simpler to test). No need to look dynamically for them, we can generate them at compile time.

[ultimate-deej]

Ok, will add some tests/samples.
Generating bundlers at compile time is a good idea. The only thing I'd change is to create a single class to store static bundler instances. Otherwise we could have multiple instances of the same bunlder.

[frankiesardo]

Unless it makes the code easier to write and to read, I wouldn't bother with generating a Master class. Are there any other benefits a part from less memory consumption?

[ultimate-deej]

Creating an instance for every call is obviously wrong. Instead, we want a single instance for the whole lifetime of an app.
Having one instance per helper is better than nothing, but it seems to be a half measure to me. Plus this kinda breaks the initial idea.

[frankiesardo]

My approach is to make it work the simplest solution first and then iterate on that. For this PR a static instance per helper is enough.

[ultimate-deej]

Going to try it anyway, I don't think it will be much harder.

[ultimate-deej]

I've changed it to store instances in a single file.
But if you use it in a library, there are two classes with the same name. The class has constant name icepick.Icepick$$Bundlers. How do you think I can workaround it? All I can think of is adding some kind of unique suffix to its name every time the class is generated. But maybe you know a better way?

Also I want to clarify something regarding tests. Do they merely check the contents of generated files?

[frankiesardo]

I can't think of a clean way to make it work with libraries. Generating a random suffix would make the test harder to write and needlessly complicate the implementation. My advice is (at the cost of sounding like a broken record) make it work with static members first.

I'm not fully understanding your question about the tests. The Truth assertions check that the input files compiles without errors and generates the corresponding output classes. I think the check is slightly more sophisticated than just string equality (it might be at bytecode level tho I'm not sure).

[ultimate-deej]

My advice is (at the cost of sounding like a broken record) make it work with static members first.

I'm not sure if I understand. In the latest revision bundlers are stored as static final fields in icepick.Icepick$$Bundlers class.

[johncarl81]

API looks good. 👍

In terms of the bundler variables, what about just creating them new for each getWithBundler() and putWithBundler call? This would enforce that they contain minimal state as the given StateBundler's lifecycle would begin and end within the respective calls. Also, there would be no issues around variable name collisions. The drawback is incurring a new for each call, although I'm not sure if this is egregious.

[ultimate-deej]

Personally, I'm comfortable with a hash map. Calls to Icepick are pretty rare, and they are not very performance-sensitive (at all if you ask me). So I'd revert to the initial implementation, as it will allow to use it in libraries, while not affecting performance much.

[johncarl81]

The BUNDLERS map in a75e9a0? I guess that work work, but it does make a mapping between a given class and a bundler. So, in other words, mapping the same type to different bundlers wouldn't work.

[ultimate-deej]

@johncarl81 It is a bundler class to bundler instance map

[MarcinOrlowski]

I dare to say I am not the only one waiting this PR merged so finally I could have both Parceler and IcePick in the projects. Hope current stagnation is just temporary. Thanks for all the efforts guys.

[frankiesardo]

This PR is not finished but I'm going to pick it up from here

[frankiesardo]

Pushed 3.2.0

[ultimate-deej]

Thank you