-
-
Notifications
You must be signed in to change notification settings - Fork 382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
play-contrib: Various improvents #1332
Conversation
Fixed visibility of various targets. Made the use of the RouteCompileWorkerModule (as ExternalModule) customizable. Introduced new logger parameter in mill.api.Classloader.create.
But keept the setup prepared to easily un-share it for future Play versions.
sharedPrefixes = sharedPrefixes, | ||
logger = None) | ||
} | ||
|
||
def create(urls: Seq[URL], | ||
parent: java.lang.ClassLoader, | ||
sharedLoader: java.lang.ClassLoader = getClass.getClassLoader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this default argument is the dangerous part, should this be deprecated too?
I expect the deprecated method have this default value so the old code use the deprecated method and get the deprecation notice. Since you're defaulting the logger to None
I think the old code is using this method directly.
Can you explain what's going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changed method is source-compatible to the old one, because the new argument (the logger) has a default value. But it's not binary compatible, so we need an overload with the old signature. But in Scala only one overload can have default arguments, that's why the deprecated method no longer has default arguments, but the new one has.
The default value for the sharedLoader
is ok as long as you don't need to share classes not already visible to mill core. This is true for in most cases (e.g. sbt-test-interface, jna, scala{_,js,native} worker api), but not in case of playlib.
The motivation to change this method was solely the wish to log the shared classloading when using mill with --debug
option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you're probable right, we should take the opportunity to remove the default for the classloader, as even if it works now, it may hinder us later, when we want to change packaging, embedding or JVM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is a big footgun and can't be checked by our tests which makes it even scarier. But removing it would change the behaviour of other modules using it, unless you expose a deprecated classloader
here for the other modules to consume.
As usual, a decision important enough to be tackled in a separate PR; as you said the deprecation here is only about the logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Moved and renamed some classes for clarity.
Fixed visibility of various targets.
Consolidated declared packages.
Made the use of the
RouteCompileWorkerModule
(as ExternalModule) customizable.Introduced new
logger
parameter inmill.api.Classloader.create
.Removed code duplication in Play-version specific workers.
But kept the setup prepared to easily un-share it for future Play versions.