-
Notifications
You must be signed in to change notification settings - Fork 74
Expose getVersionDetails() method on GitVersionPlugin #1156
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |||||||||||||||||||||||||||||
| package com.palantir.gradle.gitversion; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import groovy.lang.Closure; | ||||||||||||||||||||||||||||||
| import javax.inject.Inject; | ||||||||||||||||||||||||||||||
| import org.gradle.api.Action; | ||||||||||||||||||||||||||||||
| import org.gradle.api.Plugin; | ||||||||||||||||||||||||||||||
| import org.gradle.api.Project; | ||||||||||||||||||||||||||||||
|
|
@@ -25,13 +26,20 @@ | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| public final class GitVersionPlugin implements Plugin<Project> { | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| private Project project; | ||||||||||||||||||||||||||||||
| private Provider<GitVersionCacheService> serviceProvider; | ||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's call this |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @Inject | ||||||||||||||||||||||||||||||
| public GitVersionPlugin(Project project) { | ||||||||||||||||||||||||||||||
| this.project = project; | ||||||||||||||||||||||||||||||
| this.serviceProvider = GitVersionCacheService.getSharedGitVersionCacheService(project); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||
| @SuppressWarnings("HiddenField") | ||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't think this applies anymore? |
||||||||||||||||||||||||||||||
| public void apply(final Project project) { | ||||||||||||||||||||||||||||||
|
Comment on lines
+39
to
40
|
||||||||||||||||||||||||||||||
| project.getRootProject().getPluginManager().apply(GitVersionRootPlugin.class); | ||||||||||||||||||||||||||||||
|
Comment on lines
+35
to
41
|
||||||||||||||||||||||||||||||
| this.serviceProvider = GitVersionCacheService.getSharedGitVersionCacheService(project); | |
| } | |
| @Override | |
| @SuppressWarnings("HiddenField") | |
| public void apply(final Project project) { | |
| project.getRootProject().getPluginManager().apply(GitVersionRootPlugin.class); | |
| } | |
| @Override | |
| @SuppressWarnings("HiddenField") | |
| public void apply(final Project project) { | |
| project.getRootProject().getPluginManager().apply(GitVersionRootPlugin.class); | |
| this.serviceProvider = GitVersionCacheService.getSharedGitVersionCacheService(project); |
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 think we have to be careful here. It's not clear what will be selected when being referenced from buildscripts as getVersionDetails will be accessible via versionDetails in buildscripts (potentially).
I think the number of places where this actually works is limited since you have to get the result of the plugin application.
But now you have two methods that look very similar, but one returns a realised VersionDetails and the other returns a Provider<VersionDetails> . If I were scanning the code to see how something worked, I would not be surprised if I landed on this method, and got confused at what looks like magic, despite it actually being the closure method.
To resolve this, I think we should either:
- change the name of the method
- just get what we need by using the cache service directly instead.
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.
also this method is not the same as what you get in the closure. You're always passing null to the details. IIRC this method was added for the internal frontend monorepo, so it's also not clear whether this gets you want you want.
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.
final