-
Notifications
You must be signed in to change notification settings - Fork 96
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
Generate grgit extension for Gradle Kotlin DSL #292
Conversation
bc3c61e
to
7f464b4
Compare
I'll take a look at merging this in the next release. Thanks! |
prj.logger.warn("Project ${prj.path} already has a grgit property. Remove org.ajoberstar.grgit from either ${prj.path} or ${project.path}.") | ||
} | ||
prj.ext.grgit = grgit | ||
prj.extensions.add(Grgit,"grgit", grgit) |
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.
Nit: Could / should use single quotes for the non-interpolated string 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.
Is there some sort of performance improvement using one vs the other?
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 would guess so, as single-quoted strings to not need to be parsed for variables to expand, but I have no numbers to underpin that, and for sure it is a micro-optimization, but nevertheless good practice, IMO.
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.
Just for the record: Single quotes is the preferred style but only microscopic difference performance wise. There are a few extra cycles needed early on during parsing where no placeholders will be found (in a case like this) and both become the same thing after that. The slight preference is for human readers knowing that don't need to look ahead for dollar placeholders.
I've cherry-picked your commit with a few addendums into #302. Made sure to keep your authorship, so you still get the credit. Thanks! |
prj.logger.warn("Project ${prj.path} already has a grgit property. Remove org.ajoberstar.grgit from either ${prj.path} or ${project.path}.") | ||
} | ||
prj.ext.grgit = grgit | ||
prj.extensions.add(Grgit,"grgit", grgit) |
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.
There might be some behavior change between ext
vs extensions
.
We had some places where we added grgit
plugin multiple times. It was working fine before. With this change, grgit
within subprojects becomes null
.
Applying only at the root level solves it.
The issue is that it is really hard to debug. Here is the error we get:
> Cannot invoke method fetch() on null object
In order for the Gradle Kotlin DSL to properly generate statically typed extensions plugins must use the
ExtensionContainer
instead of just setting the value on theExtraPropertiesExtension
.