search

ph4r05 / javacard-gradle-plugin

Gradle plugin for JavaCard development
MIT License
9 stars 8 forks source link

Kotlin DSL compatibility #1

Open Vampire opened 3 years ago

Vampire commented 3 years ago

Please improve Kotlin DSL compatibility. Currently you have to use a lot of delegateClosureOf calls, as all the extension hierarchy expects Closure arguments. It would already be much better if you additionally provide Action<...> variants of those methods. Actually you could also make the methods expect Action<...> instead of Closure as both Groovy closures and Kotlin lambdas can implement Action but that would be a breaking change.

Even better would of course be if you move toward best practices, not having configuration methods like JavaCard.config() but register Config as extension of JavaCard and so on, and more importantly making the extension (and tasks) use the property / provider api for their fields, enabling easy lazy configuration and binding, and removing the need to do afterEvaluate tricks. That would of course be a big breaking change though.

ph4r05 commented 3 years ago

Thanks for suggestion @Vampire!

However, I am not using Kotlin DSL in my gradle projects and as this is my OSS free-time project I don't have a motivation to work on this refactoring given other priorities.

I may look into this later (could be months), initial suggestions seems helpful. Or feel free to submit a PR.

Thanks!

ph4r05 commented 3 years ago

I could test the Action<> change.

Vampire commented 3 years ago

The Action change I could easily PR, that costs about 5 minutes to do. Just tell me if you want the action parameters to replace the closure parameters which will be a source compatible change but not a binary compatible change or additional action overloads.

The other changes, using the property / provider lazy api, not using afterEvaluate, and so on actually has nothing to do with Kotlin DSL compatiblility, just with bad / obsolete practices, reliability and error-proneness. Those are things you should consider even when only supporting Groovy DSL.

Vampire commented 3 years ago

At #3 you find a PR with the action changes that should keep binary compatibility. If you don't care about binary compatibility but only about source compatibility, then the closure variants can simply be deleted as Groovy closures can also implement the action interface.

ph4r05 commented 3 years ago

Thanks for PR! Quick question - why are config classes now abstract?

Vampire commented 3 years ago

Because they have an abstract method that will be implemented by Gradle magic to inject the object factory. The object factory is used to instantiate the config classes to add some more Gradle magic for the @HasImplicitReceiver magic of Action. Without that you would have to do

config {
    it.debugGpPro = true
}

instead of the intended

config {
    debugGpPro = true
}
ph4r05 commented 3 years ago

Aah great, that sounds helpful, thanks @Vampire !

ph4r05 commented 3 years ago

Even better would of course be if you move toward best practices, not having configuration methods like JavaCard.config() but register Config as extension of JavaCard and so on, and more importantly making the extension (and tasks) use the property / provider api for their fields, enabling easy lazy configuration and binding, and removing the need to do afterEvaluate tricks. That would of course be a big breaking change though.

This sounds like a big piece of work and honestly I have no idea how that would work. So I leave it for some experimental PRs.