Add io.nuun.kernel as default package root

nuun-io / kernel

Nuun Kernel is an IOC component for creation of powerfull enterprise stack.

Other

8 stars 5 forks source link

Add io.nuun.kernel as default package root #58

Closed pith closed 8 years ago

pith commented 8 years ago

Improve build time by a factor of 2 on the Kernel.

ejemba commented 8 years ago

@pith I'm wondering if this restriction is not too restrictive. I mean if a project is using nuun and does not configure any package root path it will never scan anything. The default behaviour change radically currently the behaviour is to allow classpath scan even if we scan the whole classpath, with this PR nothing will happen ... Am I wrong ? If no can we imagine another solution ? If I go for the META-INF/nuun/root/io.nuun (an empty file with the package root) convention this can help for the IT tests by putting this in src/test/resources/META-INF/.

pith commented 8 years ago

Well yes, it will be very restrictive and this is a breaking change. But I'm not sure what is better between: no scan by default or an OutOfMemoryError.

None of these issues are obvious to debug for a newcomer.

Maybe we can add a method packageRoot(String...) on KernelConfiguration and a META-INF/resources/nuun.conf file (Or a yaml file ?).

ejemba commented 8 years ago

Regarding KernelConfiguration.packageRoot(...) yes we can start to add it, we can achieve the same with KernelConfiguration.param(String key, String value) today but packageRoot() will be easier in term of devX.

I suggest 2 things :

Have the hability to setting the information inside a configuration stuff like /META-INF/resources/nuun.conf (I prefer avoiding yaml to avoid an external library). standard java stuff should be enough for nuun.
Displaying a big/annoying warning when using nuun without packageRoot

========================== WARNING ==========================
Please update your application package root inside 
     → /META-INF/resources/nuun.conf
with 
     →  nuun.package.root = com.acme1, com.acme2

You're actually scanning the WHOLE classpath 
========================== WARNING ==========================

the warning could be removed via option/ ENV variable etc

What do you think @pith ? this should help adopting the parameter.

pith commented 8 years ago

Good for me. @adrienlauer what do you think about this ?

adrienlauer commented 8 years ago

I like the idea of KernelConfiguration.packageRoot(...), it's easier to understand/discover than a kernel parameter.

If we are only scanning the io.nuun package, it is indeed necessary to add a warning. If the developer want to scan the whole classpath anyway, it could use a method like KernelConfiguration.allPackages() to disable it.

Regarding, the nuun.conf file, I think it should be placed at the root of the classpath and certainly not under META-INF/resources as this location is served over HTTP in a Servlet environment.

ejemba commented 8 years ago

KernelConfiguration.packageRoot(...) → Check

We have two options

no default package so scanning everything + warning telling that you should narrow the classpath scan scope using an option or programatically.
default package set to io.nuun + warning telling that you should add an option to configuration.

I personally prefer the first option because in term of experience in the second option the user will be in a position where its code actually won't work nothing will be scanned. In the first, at least it will work in the first place , the warning will tell him what to do. In nuun project test/resources/nuun.conf can contains the package root updated to io.nuun .

@adrienlauer @pith

pith commented 8 years ago

LGTM

pith commented 8 years ago

I created 2 new isses for what we discussed here.