Replace consumer methods with Gradle Action

[SUPERCILEX]

Motivation

The Action interface is universal across Gradle API (Consumer is not) and this library is supposed to be Gradle centric. This matters because Actions are annotated with @HasImplicitReceiver, thus improving syntax for languages like Kotlin.

Old:

Grgit.clone {
    it.uri = "..."
    it.x
    it.y
}

New:

Grgit.clone {
    uri = "..."
    x
    y
}

[ajoberstar]

Thanks for the interest in the library! While Gradle builds are the biggest consumer (probably) of grgit, it is not meant to be "Gradle-centric". I want to ensure that grgit-core is still usable outside of Gradle to allow for other use cases.

Not being very familiar with Kotlin myself, are there other options to provide the syntax improvement without adding a hard Gradle dependency to core?

[SUPERCILEX]

Not unless you want to add a dependency to Kotlin and write a compiler extension. 😉

What if we added back the consumer overloads, but suffixed with j. So add -> addJ. Or is there some way I could split the AST transformations into one for the Gradle module and another for a new Java module?

[ajoberstar]

Sorry for the delay... I'm not sure about the splitting option, but that might be worth looking into. If we can put this Action specific transform in the gradle module, maybe that would work out.

[SUPERCILEX]

@ajoberstar Holy crap, I think I actually figured it out! 😁 This PR is about to get way bigger so I apologize in advance.

[SUPERCILEX]

@ajoberstar So here's the gist of it:

• AST transformations are processed when compiling the module for the target runtime (Gradle or plain old Java)
• That means the grgit-core module has no valid entry points
• This change is backwards compatible ✔️
• Unfortunately, that means there's quite a bit of boilerplate. The benefit is that in the future, we can add transformations specific to just Gradle (or just Java) if new APIs come out.

[ajoberstar]

@SUPERCILEX Thanks for all of the work you put into this! However, I think I may have misread your previous comment (emphasis mine).

Or is there some way I could split the AST transformations into one for the Gradle module and another for a new Java module?

I was hoping there would be some way to introduce something to the Gradle module that provides the necessary extensions there without having to necessitate significant change to the existing core module.

With the updated PR, I'm concerned the structure with base, java, and gradle modules will add a larger amount of overhead to future changes than I'm comfortable with taking on.

As a library targeted at providing Groovy bindings to JGit (often in the context of Gradle), Gradle's Kotlin support has me unsure of if/how to change to accommodate.

If we generate Action bindings (in addition to Consumer ones) do calls fail due to ambiguity of the two interfaces?

[SUPERCILEX]

OMG, your comment sparked an idea and it worked. I feel really stupid now 🤦‍♂️. I guess I learned a lot. 😁

Anyway, TL;DR is that this PR will only be a few lines of code in the end. 👍

[SUPERCILEX]

@ajoberstar I'm running into a weird error with failing tests whenever I added the gradleApi() dependency. It's unrelated to my changes and you can replicate it on master:

java.lang.RuntimeException: Unsupported type (org.ajoberstar.grgit.Person) found for field 'author' while constructing immutable class org.ajoberstar.grgit.Commit.
Immutable classes only support properties with effectively immutable types including:
- Strings, primitive types, wrapper types, Class, BigInteger and BigDecimal, enums
- classes annotated with @KnownImmutable and known immutables (java.awt.Color, java.net.URI)
- Cloneable classes, collections, maps and arrays, and other classes with special handling
  (java.util.Date and various java.time.* classes and interfaces)
Other restrictions apply, please see the groovydoc for ImmutableOptions for further details
	at org.codehaus.groovy.transform.ImmutableASTTransformation.checkImmutable(ImmutableASTTransformation.java:382)
	at org.ajoberstar.grgit.util.JGitUtil.convertCommit(JGitUtil.groovy:120)
	at org.ajoberstar.grgit.operation.CommitOp.call(CommitOp.groovy:78)
	at org.ajoberstar.grgit.internal.OpSyntax.mapOperation(OpSyntax.groovy:19)
	at org.ajoberstar.grgit.operation.BranchAddOpSpec.setup(BranchAddOpSpec.groovy:20)

I've added a hack in the meantime.

[SUPERCILEX]

Lookin' sexy!

[SUPERCILEX]

@ajoberstar I correctly fixed the test errors (gradleApi() isn't included in the test classpath)

[ajoberstar]

Apologies for the delayed response. That error message is a Groovy 2.5 incompatibility (which is the reason I had to split the code into two modules). With the gradleApi() dependency in place, it compiles against Groovy 2.4, which prevents Grgit from being used with Groovy 2.5 (#237).

I do like the idea to introduce a new interface that can have that @HasImplicitReceiver annotation.

However, I want to retain the Groovy 2.5 support, so we'll need to find another way to have both @HasImplicitReceiver on the classpath and Groovy 2.5. (The gradleApi dependency very frustratingly doesn't behave like any other dependency, so normal conflict resolution doesn't help.)

One other note... There are a lot of extraneous changes in the PR right now (some formatting changes and code cleanup). There's nothing wrong with those on their own, but since they're unrelated to the code that needs changing, I'd like them removed from the PR..

[SUPERCILEX]

@ajoberstar Cool, makes sense. 👍 For the extraneous changes, I've moved them to #257. I'll look into the Groovy 2.5 issues.

[SUPERCILEX]

@ajoberstar Ok, I got it working by injecting the annotation in an AST transformation. 😁

Will remove the extraneous changes once #257 goes through.

[ajoberstar]

Sorry for the delay. This new approach seems pretty promising. Could you cut this down to a single commit with just the OpSyntax change, new Configurable annotation, and related AST? I'd like to include it for the 3.0 release I hope to cut soon.

[SUPERCILEX]

@ajoberstar Awesome! Could you merge cleanup anyway? It would make my life way easier since this PR depends on it. cleanup also includes some changes that wouldn't be solved by Spotless. If you really don't want to merge it, I'll do my best to rip cleanup out of this PR.

[ajoberstar]

For now, all I'd like to pull in is the change in the AST generated method. I've made a new PR #262, which takes just those changes without the cleanup. I appreciate all the work you did on this, so I made sure to mark you as the author of the commit.

I'll be closing this PR in favor of the new one. Thanks again!

[SUPERCILEX]

@ajoberstar Wow, thanks for doing all that work and keeping me as the author! ❤️