Enhance Ginjector generation #77
Conversation
|
ArcBees » GWTP #100 FAILURE |
|
ArcBees » GWTP #101 SUCCESS |
|
Could you format to <120 characters. I noticed a few lines overshoot :) |
| public void setPresenters(Set<JClassType> presenters) { | ||
| this.presenters = presenters; | ||
| } | ||
| } No newline at end of file |
|
Nice job. Other than my formatting nits, this looks good to me :) |
|
Do you think a unit test could be used on this? |
|
I will fix the formatting tomorrow. |
| * providers[ID_Object2] = object2Provider; | ||
| * } | ||
| * }</pre> | ||
| * ProviderBundles are automatically generated by GWTP. |
There was a problem hiding this comment.
Bring back the old documentation please, it's still an option to use the generated generators.
There was a problem hiding this comment.
Ok, how to you want to handle the ProxyCodeSplitBundle annotation, should it be extended with a third parameter or should there be a new annotation for generated bundles.
There was a problem hiding this comment.
What do you mean? Is your change backward compatible?
There was a problem hiding this comment.
At the moment not, that is why I ask.
There was a problem hiding this comment.
It should be backward compatible, we don't want to brake other users :D So yes, we have to look at what we can do to make this happen. I don't remember, why a third parameter was needed?
There was a problem hiding this comment.
Ok, maybe it's my brain that is forgetting how to use ProviderBundle since it's something I never had to use until now... but the goal is only to write the ginjector, what would usually be in the ginjector when there's a provider bundle in the application?
There was a problem hiding this comment.
Well usually you create your bundle class and create id's for all the presenters that are supposed to be in the bundle, this is a really piece of work to keep up-to-date.
The injector only has one line per bundle with an AsyncProvider.
My implementation generates all the bundles that are necessary with out the need for the developer to worry about things like indexes and bundles sizes. Maybe my bad cause I mixed this with the ClientGinjector generation issue but I really see it as one problem.
There was a problem hiding this comment.
So true!
That's a bonus then 💃
Well, I think the only thing now is to make sure that it's backward compatible, aside of that, it's a really great feature that you added!
There was a problem hiding this comment.
Yea, i'd see we just extend the current annotation with a string value that identifies the bundle.
You can then use either bundeClass and id or just the value, other combinations need to be prevented by the PresenterInspector.
public @interface ProxyCodeSplitBundle {
Class<? extends ProviderBundle> bundleClass();
int id();
String value();
}There was a problem hiding this comment.
alright, let's go with that, can you write some documentation about it? It will at least need to be documented in the javadoc, but If you can write a little something for users that just started to use the framework, it will help them a lot
|
Need integration test in fact. One of the sample should be updated to make sure that it's still running and at some point we should fully test team using cucumber |
|
Good job! I'll take on the task for the Gatekeepers as well as to introduce a Bootrapper sequence |
|
Don't worry for the test, I want all sample to use cucumber's test at some point, we'll take that part on. If you have time, in a different PR, please add your work to one sample that has bundles and/or gatekeepers. Thanks! |
|
I have this running in a pretty big app so it works, I will update a sample in the next few days. |
|
ArcBees » GWTP #102 SUCCESS |
| ClassSourceFileComposerFactory composer = initComposer(); | ||
| SourceWriter sourceWriter = composer.createSourceWriter(generatorContext, printWriter); | ||
|
|
||
| writeInit(sourceWriter); | ||
| writeInit(sourceWriter, | ||
| new GinjectorGenerator().generate(getTreeLogger(), generatorContext, GinjectorGenerator.DEFAULT_FQ_NAME)); |
There was a problem hiding this comment.
Extract GinjectorGenerator().generate to a variable for better readability
|
Two small last nits :D Then LGTM! |
|
Will fix the final stuff tomorrow and maybe add a complex sample with Gatekeepers. One thing that I noticed in our project was the TabInfo annotation. Can not really provide the Ginjector as parameter any more. Any idea on how to solve this? |
|
Uhm this one is tricky, but it could become an optional parameter so that if none is found, it use the generated generator which always have the same name. |
|
Well it always has the same name but you don't really have access to it's methods. Maybe we can allow an arbitrary parameter list of any gin-bound classes like gatekeepers and clientbundles somehow? |
|
Yeah, well seems like the ginjector interface that you added at first will still be needed in some cases. But I would rather have the ClientGinjector extends the user ginjector defined accessors. What I don't like is that we will have to check every type to make sure that we don't double bind anything. |
|
@TabInfo now accepts multiple parameters either of a custom ginjector type or any type that is provided through the ginjector, this allows for parameters such as gatekeepers or message resources. |
|
ArcBees » GWTP #105 SUCCESS |
|
ArcBees » GWTP #106 SUCCESS |
|
Ready to merge? |
|
I heard its not so we can wait till your back after holidays Peter. Thanks. :) |
|
Afaik it is ready. I can rebase tomorrow if that is ok.
|
|
No problem take your time :) |
|
LGTM! :D |
|
Ok.... the rebase messed up the commit count, file changes are still the same. Probably should have merged :( |
|
I messed this one up... |
|
Ok now it is ready. |
|
ArcBees » GWTP #135 SUCCESS |
|
Good, next step will be to update the sample! Good job! I'll try to release an alpha 2 in a few days. Thanks again and merry Christmas! |
|
Two of the samples are updated in ArcBees/GWTP-Samples#9 Merry Christmas to you as well! |
|
Merry Christmas. Thanks for everything. :) |
Enhance Ginjector generation
Enhance Ginjector generation
Enhance Ginjector generation Former-commit-id: 0b9b65d
Added the ability to use Gatekeepers with ApplicationController generation.
You can now add an additional interface to the generated Ginjector that allows for example the declaration of a default gatekeeper.
Fixed the use of ProviderBundles in the generated Ginjector and added a generator that generates all ProviderBundles. No more manually keeping the presenter id's and bundle size in order.
Please review :)