add properties to gradle profile #9207

renanfranca · 2024-03-13T13:34:18Z

See

Support setting build property in gradle #8628

codecov · 2024-03-13T13:43:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e178946) to head (1a4f0c5).
Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##                main     #9207   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity      2928      2940   +12     
===========================================
  Files            733       734    +1     
  Lines          12649     12681   +32     
  Branches         256       257    +1     
===========================================
+ Hits           12649     12681   +32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

murdos · 2024-03-13T16:34:05Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+        if (!scriptPlugin.exists()) {
+          throw new MissingGradleProfileException(buildProfile);
+        }
+        addPropertyToProfile(command.property(), buildProfile);


How about doing directly:

Suggested change

addPropertyToProfile(command.property(), buildProfile);

addPropertyTo(command.property(), scriptPlugin);

You've just resolved the script plugin path, there's no need to resolve it again :)

Thanks 😁!

murdos · 2024-03-13T16:41:16Z

...ite/module/infrastructure/secondary/javadependency/gradle/MissingGradleProfileException.java

+  public MissingGradleProfileException(BuildProfileId profileId) {
+    super(
+      badRequest(GradleDependencyErrorKey.MISSING_PROFILE).message(
+        "Your gradle project does not contain the profile-%s.gradle.kts file".formatted(profileId)


Suggested change

"Your gradle project does not contain the profile-%s.gradle.kts file".formatted(profileId)

"Your gradle project does not contain any "profile-%s.gradle.kts" precompiled script plugin".formatted(profileId)

murdos · 2024-03-13T16:43:24Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+  }
+
+  private void addProperty(BuildProperty property, String buildGradleProfileFilePath) {
+    String gradlePropertyFormatted = convertToKotlinFormat(property);


Suggested change

String gradlePropertyFormatted = convertToKotlinFormat(property);

String gradlePropertyDeclaration = propertyKotlinDeclaration(property);

murdos · 2024-03-13T16:59:40Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+    return "val %s by extra(\"%s\")".formatted(convertToKotlinFormat(property.key()), property.value().get());
+  }
+
+  private String convertToKotlinFormat(PropertyKey key) {


This naming seems more explicit:

Suggested change

private String convertToKotlinFormat(PropertyKey key) {

private String toCamelCasedKotlinVariable(PropertyKey key) {

murdos · 2024-03-13T17:02:38Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+
+  private void addProperty(BuildProperty property, String buildGradleProfileFilePath) {
+    String gradlePropertyFormatted = convertToKotlinFormat(property);
+    Optional<String> propertyLine = readPropertyFrom(convertToKotlinFormat(property.key()), buildGradleProfileFilePath);


Do you really need read the existing file?
Using a regexp like val %s by extra\\(([^)])\)".formatted(convertToKotlinFormat(property.key()) could probably work.

Thank you for the review! Interesting, I will try it 👍

I am thinking here, let me share with you. What I need to know is if the property already exists to use a RegexReplacer instead of the RegexBefore. So I need to read the file to know if the property exists, but I dont need anymore to seek for the exact line if I use this Regex. WDYT?

Oh, yeah, you're probably right. Let's keep it that way then 👍

@murdos : I did as we agreed: (improve existing property replace). But then I thought on a solution and implemented it too check if UnknownCurrentValueException throws instead of read the gradle profile file to know if a property did not exist.

@renanfranca : I'm not convinced by e642e94 : relying on (a runtime) exception to adjust behavior is not a good idea, and is not really robust.
A runtime exception is not an explicit API: if FileReplacer changes its behavior and throw a different exception, or stop throwing this exception, that would break GradleCommandHandler unexpectedly.

Thank you for the careful look and constructive review, I totally agree with you. I am going to drop this commit now 👍

murdos · 2024-03-13T17:05:14Z

src/main/resources/messages/errors/generator-errors-messages_fr.properties

@@ -32,6 +32,9 @@ error.missing-java-build-configuration-files.detail=Aucun fichier de configurati
 error.invalid-toml-version-catalog-file.title=Catalogue de version Gradle TOML invalide
 error.invalid-toml-version-catalog-file.detail=Le fichier gradle/libs.versions.toml est invalide

+error.missing-gradle-profile.title=Profil Gradle manquant
+error.missing-gradle-profile.detail=Votre projet gradle manque du fichier de profil cible


Suggested change

error.missing-gradle-profile.detail=Votre projet gradle manque du fichier de profil cible

error.missing-gradle-profile.detail=Votre projet Gradle ne contient pas de fichier correspondant au profil

murdos · 2024-03-15T08:21:00Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+  }
+
+  private static MandatoryReplacer addNewPropertyReplacer(String gradlePropertyDeclaration) {
+    return new MandatoryReplacer(new RegexNeedleBeforeReplacer(always(), GRADLE_PROPERTY_NEEDLE), gradlePropertyDeclaration);


The API would be more explicit if the parameter would be the BuildProperty, and if you were doing that (calling method convertToKotlinFormat()):
return new MandatoryReplacer(new RegexNeedleBeforeReplacer(always(), GRADLE_PROPERTY_NEEDLE), convertToKotlinFormat(property));

murdos · 2024-03-15T08:22:09Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

@@ -308,22 +320,14 @@ private String toCamelCasedKotlinVariable(PropertyKey key) {
  }

  @ExcludeFromGeneratedCodeCoverage(reason = "The exception handling is hard to test and an implementation detail")
-  private Optional<String> readPropertyFrom(String gradlePropertyFormatted, Path buildGradleProfileFile) {
+  private Boolean propertyExistsFrom(String gradlePropertyFormatted, Path buildGradleProfileFile) {


You should use primitive boolean: that removes the 3rd possible value: null

Also here the API would be clearer if you use the BuildProperty as parameter rather than String gradlePropertyFormatted

…er organization

murdos · 2024-03-15T11:23:58Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+    try {
+      String content = Files.readString(buildGradleProfileFile);
+
+      return content.contains(toCamelCasedKotlinVariable(key));


It might be more robust to verify the variable declaration, rather than just the variable appearance, no?

Suggested change

return content.contains(toCamelCasedKotlinVariable(key));

return content.contains("val %s by extra(".formatted(toCamelCasedKotlinVariable(key));

You are right! Only the key could find a name out of context.

murdos · 2024-03-15T11:24:48Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+
+      return content.contains(toCamelCasedKotlinVariable(key));
+    } catch (IOException e) {
+      throw GeneratorException.technicalError("Error writing pom: " + e.getMessage(), e);


Exception message should be updated, there's no pom involved :-)

Sorry, copy and paste is a hell 😅

murdos · 2024-03-15T11:26:02Z

...hipster/lite/module/infrastructure/secondary/javadependency/gradle/GradleCommandHandler.java

+
+  private void addPropertyTo(BuildProperty property, File buildGradleFile) {
+    MandatoryReplacer replacer;
+    if (propertyExistsFrom(property.key(), buildGradleFile.toPath())) {


👍 The code is now clean and easy to read and understand :-)

Thanks murdos 🎉! I have to use Value Objects as much as I can 😁

… and rename parameter for a better description

renanfranca added area: feature request 💡 theme: gradle labels Mar 13, 2024

renanfranca self-assigned this Mar 13, 2024

renanfranca mentioned this pull request Mar 13, 2024

Support setting build property in gradle #8628

Closed

2 tasks

renanfranca requested a review from murdos March 13, 2024 13:47

renanfranca force-pushed the 8628-add-properties-to-gradle-profile branch from 8f852ea to 93c1f73 Compare March 13, 2024 14:27

murdos reviewed Mar 13, 2024

View reviewed changes

renanfranca force-pushed the 8628-add-properties-to-gradle-profile branch from 93c1f73 to e642e94 Compare March 14, 2024 14:08

renanfranca requested a review from murdos March 14, 2024 14:39

murdos reviewed Mar 15, 2024

View reviewed changes

renanfranca force-pushed the 8628-add-properties-to-gradle-profile branch from e642e94 to 92a53cd Compare March 15, 2024 10:17

renanfranca added 8 commits March 15, 2024 07:21

move private methods immediately after related public method for bett…

6e2f466

…er organization

support add properties to gradle profile file

8bc2c58

avoid code duplication

71f5ecd

remove redundant method call

81bf04e

improve variable and method names

88fb784

improve error messages

9890988

improve existing property replace

282e988

improve code readability

16fa215

renanfranca force-pushed the 8628-add-properties-to-gradle-profile branch from 92a53cd to 16fa215 Compare March 15, 2024 10:23

renanfranca requested a review from murdos March 15, 2024 10:43

murdos previously approved these changes Mar 15, 2024

View reviewed changes

fix error message and improve property detection in build gradle file…

1a4f0c5

… and rename parameter for a better description

renanfranca dismissed murdos’s stale review via 1a4f0c5 March 15, 2024 11:59

renanfranca enabled auto-merge March 15, 2024 12:02

renanfranca merged commit 5737c6f into jhipster:main Mar 15, 2024
37 checks passed

murdos mentioned this pull request Mar 18, 2024

create agnostic javabuildproperties for gradle and maven #9267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add properties to gradle profile #9207

add properties to gradle profile #9207

renanfranca commented Mar 13, 2024

codecov bot commented Mar 13, 2024 •

edited

murdos Mar 13, 2024

renanfranca Mar 13, 2024

murdos Mar 13, 2024

murdos Mar 13, 2024

murdos Mar 13, 2024

murdos Mar 13, 2024

renanfranca Mar 13, 2024

renanfranca Mar 13, 2024 •

edited

murdos Mar 13, 2024

renanfranca Mar 14, 2024

murdos Mar 15, 2024

renanfranca Mar 15, 2024

murdos Mar 13, 2024

murdos Mar 15, 2024

murdos Mar 15, 2024

murdos Mar 15, 2024

murdos Mar 15, 2024

renanfranca Mar 15, 2024

murdos Mar 15, 2024

renanfranca Mar 15, 2024

murdos Mar 15, 2024

renanfranca Mar 15, 2024

	addPropertyToProfile(command.property(), buildProfile);
	addPropertyTo(command.property(), scriptPlugin);

	"Your gradle project does not contain the profile-%s.gradle.kts file".formatted(profileId)
	"Your gradle project does not contain any "profile-%s.gradle.kts" precompiled script plugin".formatted(profileId)

	String gradlePropertyFormatted = convertToKotlinFormat(property);
	String gradlePropertyDeclaration = propertyKotlinDeclaration(property);

	private String convertToKotlinFormat(PropertyKey key) {
	private String toCamelCasedKotlinVariable(PropertyKey key) {

	error.missing-gradle-profile.detail=Votre projet gradle manque du fichier de profil cible
	error.missing-gradle-profile.detail=Votre projet Gradle ne contient pas de fichier correspondant au profil

	return content.contains(toCamelCasedKotlinVariable(key));
	return content.contains("val %s by extra(".formatted(toCamelCasedKotlinVariable(key));

add properties to gradle profile #9207

add properties to gradle profile #9207

Conversation

renanfranca commented Mar 13, 2024

codecov bot commented Mar 13, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renanfranca Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2024 •

edited

renanfranca Mar 13, 2024 •

edited