Skip to content
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

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

musketyr
Copy link
Contributor

@musketyr musketyr commented Dec 8, 2021

this is an afford to replace the current config with the latest best practices for extensions


public class ExtensionUtil {

public static <E extends ExtensionAware, P extends ExtensionAware> E createIfMissing(Project project, ExtensionPath<P, E> path) {
Copy link

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.

e.g: https://github.com/micronaut-projects/micronaut-gradle-plugin/blob/master/src/main/java/io/micronaut/gradle/aot/MicronautAotPlugin.java#L117-L119

This creates an aot extension to the micronaut extension. The extension is itself created via the base plugin:

https://github.com/micronaut-projects/micronaut-gradle-plugin/blob/bbf99292e87ce026a5208ed0ca9d4163c73ec2fb/src/main/java/io/micronaut/gradle/MicronautBasePlugin.java#L24

This avoids convoluted logic about whether something is present or not: instead you declare your requirements: "I need the extension to be there"

Copy link
Contributor Author

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();
Copy link
Contributor Author

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

https://github.com/kordamp/kordamp-gradle-plugins/pull/496/files#diff-7eb248e07942ac8642c2618d3e65cb667d28fc87dffa45c67d740b7a9022f214R50-R53

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?

Copy link

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.

Copy link
Contributor Author

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);

Copy link
Contributor Author

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> 🤦

Copy link

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

Copy link
Contributor Author

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.

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