Skip to content

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

Closed
pith wants to merge 1 commit into
masterfrom
default-package
Closed

pith wants to merge 1 commit into
masterfrom
default-package

Conversation

@pith

@pith pith commented Mar 7, 2016

Copy link
Copy Markdown
Contributor

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

@ejemba

ejemba commented Mar 7, 2016

Copy link
Copy Markdown
Member

@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

pith commented Mar 8, 2016

Copy link
Copy Markdown
Contributor Author

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

ejemba commented Mar 8, 2016

Copy link
Copy Markdown
Member

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

pith commented Mar 9, 2016

Copy link
Copy Markdown
Contributor Author

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

@adrienlauer

Copy link
Copy Markdown
Contributor

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

ejemba commented Mar 9, 2016

Copy link
Copy Markdown
Member

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

pith commented Mar 9, 2016

Copy link
Copy Markdown
Contributor Author

LGTM

@pith

pith commented Mar 9, 2016

Copy link
Copy Markdown
Contributor Author

I created 2 new isses for what we discussed here.

@pith pith closed this Mar 9, 2016
@pith
pith deleted the default-package branch March 9, 2016 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants