-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
support dependencies and dependencies management in a build profile #9369
support dependencies and dependencies management in a build profile #9369
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9369 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 2941 2950 +9
===========================================
Files 734 734
Lines 12778 12797 +19
Branches 257 259 +2
===========================================
+ Hits 12778 12797 +19 ☔ View full report in Codecov by Sentry. |
@@ -142,15 +144,15 @@ private Pattern needleForGradleDependencyScope(GradleDependencyScope gradleScope | |||
}; | |||
} | |||
|
|||
private String dependencyDeclaration(JavaDependency dependency) { | |||
private String dependencyDeclaration(JavaDependency dependency, Optional<BuildProfileId> buildProfile) { | |||
StringBuilder dependencyDeclaration = new StringBuilder() | |||
.append(indentation.times(1)) | |||
.append(gradleDependencyScope(dependency).command()) | |||
.append("("); | |||
if (dependency.scope() == JavaDependencyScope.IMPORT) { |
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.
It's a bit a shame to use a parameter Optional<BuildProfileId>
for versionCatalogReference()
where you only use Optional::isPresent.
Maybe:
if (dependency.scope() == JavaDependencyScope.IMPORT) { | |
var versionCatalogReference = forBuildProfile ? versionCatalogReferenceForBuildProfile(dependency) : versionCatalogReference(dependency); | |
if (dependency.scope() == JavaDependencyScope.IMPORT) { |
@@ -142,15 +144,15 @@ private Pattern needleForGradleDependencyScope(GradleDependencyScope gradleScope | |||
}; | |||
} | |||
|
|||
private String dependencyDeclaration(JavaDependency dependency) { | |||
private String dependencyDeclaration(JavaDependency dependency, Optional<BuildProfileId> buildProfile) { |
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.
You don't really need the buildProfile:
private String dependencyDeclaration(JavaDependency dependency, Optional<BuildProfileId> buildProfile) { | |
private String dependencyDeclaration(JavaDependency dependency, boolean forBuildProfile) { |
@@ -55,11 +55,6 @@ public static JHipsterModule fullModule() { | |||
.pluginManagement(mavenEnforcerPluginManagement()) | |||
.plugin(mavenEnforcerPlugin()) | |||
.and() | |||
.javaDependencies() |
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.
You can now remove the gradleSupportedModule()
and gradleSupportedModuleBuilder
since we a feature parity between maven and gradle for build profile, in order to have only one module()
method.
); | ||
} | ||
|
||
private String projectFolderRelativePathFrom(Path buildGradleFile) { |
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.
How about returning directly a JHipsterProjectFilePath? You seem to always use this method that way
private String projectFolderRelativePathFrom(Path buildGradleFile) { | |
private JHipsterProjectFilePath projectFolderRelativePathFrom(Path buildGradleFile) { | |
return new JHipsterProjectFilePath(Path.of(projectFolder.folder()).relativize(buildGradleFile).toString()); |
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.
Sure! Nice 👍
@@ -330,6 +367,10 @@ private void enablePrecompiledScriptPlugins() { | |||
addFileToProject(from("buildtool/gradle/buildSrc/build.gradle.kts.template"), to("buildSrc/build.gradle.kts")); | |||
} | |||
|
|||
private void injectTomlVersionCatalogLibsIntoScriptPlugins() { |
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.
The method name is a bit misleading (you're rather initiating settings.gradle for precompiled script plugins), and I think you should rather add this addFileToProject(..)
statement directly in enablePrecompiledScriptPlugins()
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.
Thanks! Yes, It is part of the PrecompiledScript prerequisites to work properly 😃
} | ||
|
||
@ExcludeFromGeneratedCodeCoverage(reason = "The exception handling is hard to test and an implementation detail") | ||
private boolean dependencyExistsFrom(DependencySlug dependencySlug, Optional<BuildProfileId> buildProfile) { |
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.
It would be nicer to use and read with the following signature:
private boolean dependencyExistsFrom(DependencySlug dependencySlug, Optional<BuildProfileId> buildProfile) { | |
private Predicate<DependencySlug> dependencyExistsFrom(Optional<BuildProfileId> buildProfile) { | |
return dependencySlug -> { | |
try { | |
return dependencyLinePattern(dependencySlug, buildProfile).matcher(Files.readString(buildGradleFile(buildProfile))).find(); | |
} catch (IOException e) { | |
throw GeneratorException.technicalError("Error reading build gradle file: " + e.getMessage(), e); | |
} | |
}; | |
} |
That way you could use it as .filter(dependencyExistsFrom(command.buildProfile()))
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.
Thanks! I have to do this more often 👍
…t from gradle profile
dec8875
to
2c55be6
Compare
@murdos : the code coverage wasnt 100% because the annotation Thanks for the detailed review! 😁🖖 |
@renanfranca : it works for the method, but not for the lambda that is inside the method. |
@murdos: I implemented this fix: private Predicate<DependencySlug> dependencyExistsFrom(Optional<BuildProfileId> buildProfile) {
return dependencySlug -> dependencyExistsFrom(dependencySlug, buildProfile);
}
@ExcludeFromGeneratedCodeCoverage(reason = "The exception handling is hard to test and an implementation detail")
private boolean dependencyExistsFrom(DependencySlug dependencySlug, Optional<BuildProfileId> buildProfile) {
try {
return dependencyLinePattern(dependencySlug, buildProfile).matcher(Files.readString(buildGradleFile(buildProfile))).find();
} catch (IOException e) {
throw GeneratorException.technicalError("Error reading build gradle file: " + e.getMessage(), e);
}
} |
Great work, again @renanfranca ! Thanks!!! |
You are welcome! Thank you for the kind words 😄 |
See