Skip to content
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

handle properties version lookup in grails-bom #13948

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

jamesfredley
Copy link
Contributor

@jamesfredley jamesfredley commented Dec 31, 2024

test with ./gradlew assemble
unzip build/distributions/grails-7.0.0-SNAPSHOT.zip
bin/grails

Handles properties version lookup in grails-bom

https://repo.grails.org/ui/repos/tree/PomView/libs-snapshots-local/org/grails/grails-bom/7.0.0-SNAPSHOT/grails-bom-7.0.0-20241229.020850-191.pom

${profiles-angular.version} was coming from grails-bom and org.grails.cli.boot.GrailsDependencyVersions needed to be updated to handle version lookup

<properties>
     <profiles-angular.version>10.0.1</profiles-angular.versio>
</properties>
<dependencyManagement>
     <dependencies>
          <dependency>
               <groupId>org.grails.profiles</groupId>
               <artifactId>angular</artifactId>
               <version>${profiles-angular.version}</version>
          </dependency>
     </dependencies>
</dependencyManagement>

@jamesfredley jamesfredley linked an issue Dec 31, 2024 that may be closed by this pull request
Copy link
Contributor

@codeconsole codeconsole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is groovy, how about?

    String versionLookup(String version){
    	version?.startsWith('${') && version.endsWith('}') ?
        	versionProperties[version[2..-2]] : version
    }

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jamesfredley! 🚀

You could also update the license header year of GrailsDependencyVersions.

@gsartori
Copy link
Contributor

Since it is groovy, how about?

    String versionLookup(String version){
    	version?.startsWith('${') && version.endsWith('}') ?
        	versionProperties[version[2..-2]] : version
    }

The Groovy feature of not having a mandatory return is nice if the whole team agrees on that. Since we want to attract Java developers (at least this topic is in the air) I would keep return. Actually, @codeconsole, given our def/var fight I am surprised you are proposing this :)

This would be my version:

String versionLookup(String version) {
    Boolean isVariable = version?.startsWith('${') && version.endsWith('}')
    return isVariable
            ? versionProperties[version[2..-2]]
            : version
}

@matrei
Copy link
Contributor

matrei commented Dec 31, 2024

This would be my version:

String versionLookup(String version) {
    Boolean isVariable = version?.startsWith('${') && version.endsWith('}')
    return isVariable
            ? versionProperties[version[2..-2]]
            : version
}

I like this, except the capital B on Boolean.

Also, there is a null-check on the version. Is that necessary? Is null a valid value for version?

@gsartori
Copy link
Contributor

Boolean vs boolean (or any Object vs primitive type in the JVM) is an old story. I prefer to keep the code "readble and coherent" VS "I believe it will run farter". The cost of maintaining the code is immensly bigger than the benefit of a quark-improvement in performances.

The difference is nano-seconds, and not really relevant in 98% of use cases. I know we are talking religions here, but give a look here: https://stackoverflow.com/questions/6911563/java-performance-true-vs-boolean-true/6911627#6911627

@matrei
Copy link
Contributor

matrei commented Dec 31, 2024

Boolean vs boolean (or any Object vs primitive type in the JVM) is an old story. I prefer to keep the code "readble and coherent" VS "I believe it will run farter". The cost of maintaining the code is immensly bigger than the benefit of a quark-improvement in performances.

The difference is nano-seconds, and not really relevant in 98% of use cases. I know we are talking religions here, but give a look here: https://stackoverflow.com/questions/6911563/java-performance-true-vs-boolean-true/6911627#6911627

Why is Boolean more readable than boolean?
I did not suggest it because "I believe it will run faster".
I suggested it, because I believe it is the correct type.

@gsartori
Copy link
Contributor

gsartori commented Dec 31, 2024

Why is Boolean more readable than boolean?

Writing/Reading code requires mental energy (dopaminergic circuit mainly). Each decision we take requires mental energy. When we run out of it, we start makining mistakes. Mental energy (fuel) and mistakes (crashes) are "costs" from a Business point of view.

Always using type wrappers instead of deciding when to use a primitive type or an Object is one less decision to take. You just use a Type. Is less mental energy -> less cost.

On a personal note: from a style point of view it is more coherent, we are developing Object Oriented code, not a Procedural "C" like code.

@matrei
Copy link
Contributor

matrei commented Dec 31, 2024

Writing/Reading code requires mental energy (dopaminergic circuit mainly)

My mental energy loss is great when I see boxing/unboxing primitive types for no reason. I have to ask myself: why was this done?

Always using type wrappers instead of deciding when to use a primitive type or an Object is one less decision to take. You just use a Type. Is less mental energy -> less cost.

We'll have to agree to disagree on this one 😄

@gsartori
Copy link
Contributor

gsartori commented Dec 31, 2024

My mental energy loss is great when I see boxing/unboxing primitive types for no reason. I have to ask myself: why was this done?

That's because we haven't agreend on the coding style yet. Having "standards" is another argument to reduce costs and increment quality. Using only Types whould mean having less variance both in function and in style.

Occam with its Occam's Razor would have agreed with me: we would have less variables, or variance in this case, to accompish the same task.

@jamesfredley
Copy link
Contributor Author

@codeconsole Thanks, I've updated it to the groovier syntax.

@matrei I have updated to license date.

@jamesfredley jamesfredley dismissed codeconsole’s stale review December 31, 2024 16:23

Updated to groovier syntax

@jamesfredley jamesfredley merged commit 5860c82 into 7.0.x Dec 31, 2024
20 checks passed
@jamesfredley jamesfredley deleted the grails-bom-properties-version branch December 31, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distribution Fails to Run
5 participants