-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
mirror configuration model for config/info #496
base: master
Are you sure you want to change the base?
Conversation
|
||
public class ExtensionUtil { | ||
|
||
public static <E extends ExtensionAware, P extends ExtensionAware> E createIfMissing(Project project, ExtensionPath<P, E> path) { |
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.
I don't think you should have a createIfMissing
. Instead, you should have a plugin which creates the extension. Then plugins which need the extension have to apply the plugin. If it's already applied, then it's not reapplied so it's guaranteed that the extension is present. If it's not yet applied, then the extension will be created.
This creates an aot
extension to the micronaut
extension. The extension is itself created via the base
plugin:
This avoids convoluted logic about whether something is present or not: instead you declare your requirements: "I need the extension to be there"
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.
good point. I already know that the parent is present so it really makes no sense to check for presence.
public interface Repository extends Named { | ||
|
||
Property<String> getUrl(); | ||
// Property<Credentials> getCredentials(); |
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.
@melix I have some very strange behaviour here. if remove the comment here and the particular configuration here
then actually the whole extension cannot be found causing no such property kordamp
for the build script.
what is the best way how to provide
credentials {
username = 'foo'
pasword = 'bar'
}
for objects used inside NamedDomainObjectContainer
?
I've tired to let Repository
implements ExtensionAware
and made credentials
an extension but it didn't work.
and when I tried to add default methods credentials {}
then I was missing the value to delegate to and I would required ObjectFactory
to create the default values. Does having Property<Credentials>
have actually have any benefit here as the Repository
creation should be lazy itself or can I simply use Credentials
real object 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.
It's a bit difficult to tell without seeing the actual problems. There shouldn't be any problem with using a Property<Credentials>
, but it's true that you should avoid using default methods because as far as I know, default methods wouldn't automatically create the credentials { ... }
method. Wrt using Credentials
directly, I think it depends on what you want to offer as UX. For me it makes sense to use a Property
because you can then share the same instance with different repositories, while keeping track of how the credentials instance is created.
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.
and is there any simple way how to inject PropertyFactory
into the Repository
object if it's converted to the abstract class? I've tried it but it looks like it only accepts the name e.g. Repository(String name) { this.name = name; }
. Or is there any better way how to get access to the PropertyFactory
, i.g. something like PropertyFactoryAware
interface to extend so I can create a default method
default void credentials(Closure closure) {
Credentials credentials = getCredentials().convention(getPropertyFactory(}.property(new DefaultCredentials()).get();
ConfigureUtil.configure(credentials, closure);
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.
actually I'm over engineering here, right? I can set the default value as object and I don't need it as a Property<Credentials>
🤦
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.
First, don't use Closure
. Use Action
, Gradle takes care of creating the Closure
version for Groovy and the SAM receiver version for Kotlin.
Then, for creating instances in advanced use cases where you want to set the defaults for nested objects, you can take a look at how I do it in the GraalVM native plugin. It requires a bit more work since now you'd need to provide an abstract class but then it can configure defaults: https://github.com/graalvm/native-build-tools/blob/9c8271fc3dae1b9e82d51d86cecf5ae3088da4b2/native-gradle-plugin/src/main/java/org/graalvm/buildtools/gradle/internal/BaseNativeImageOptions.java#L214-L221
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.
thanks for the examples! ok so the solution would be to step back one level and create and configure the NamedDomainObjectContainer<Repository>
manually while creating the extension.
this is an afford to replace the current config with the latest best practices for extensions