Use dependency versions from the `grails-bom` where available #57

matrei · 2024-10-17T12:19:25Z

And some other improvements and cleanup.

When creating the `JavaMailSender` via constructor, it could not connect to the greenmail server.

matrei · 2024-10-17T12:20:28Z

#56

sbglasius

Two minor suggestions.

build.gradle

`groovy-templates` and `grails-docs` are also in `grails-bom`

gradle/libs.versions.toml

To restrict the use of groups.

- Remove explicit versions to where versions are available in `grails-bom` - Use `org.eclipse.angus:jakarta.mail` instead of `com.sun.mail:jakarta.mail` - Restrict the use of version catalog alias groups

`2.0.1` -> `2.1.0`

codeconsole · 2024-10-23T05:26:33Z

Well, I will ask the obvious.... now that we use the bom for versions, what is the point of using toml? doesn't it just make things more confusing and result in 2x lines of code? Does it provide anything of value?

matrei · 2024-10-23T06:59:27Z

Does it provide anything of value?

Using a version catalog has several advantages.
Most notably:

Declare dependency coordinates and versions once, and in a centralized location (DRY).
Less cluttered build files (subjective)

codeconsole · 2024-10-23T07:08:21Z

Does it provide anything of value?

Using a version catalog has several advantages. Most notably:

Declare dependency coordinates and versions once, and in a centralized location (DRY).

Less cluttered build files (subjective)

If it is anything, it is definitely NOT DRY.

uh, it looks like you are declaring them twice

grails-mail/build.gradle

Lines 24 to 54 in c0a82ec

    
           compileOnly libs.grails.core // Used in public api, but provided 
        
           compileOnly libs.groovy.core // Used in public api, but provided 
        
           compileOnly libs.servlet.api // Provided 
        
           api libs.javamail.api // Used in public api 
        
           api libs.spring.context // Used in public api 
        
           api libs.spring.context.support // Used in public api 
        
           // These two libraries are used in public api, but not in a way 
        
           // that is meant to be consumed by users of the plugin. 
        
           // Therefore they are set to implementation to not expose unnecessarily 
        
           // to the compileClasspath of plugin users. 
        
           implementation libs.grails.gsp 
        
           implementation libs.grails.web.common 
        
           implementation libs.grails.web.urlmappings 
        
           implementation libs.javamail.impl 
        
           implementation libs.spring.beans 
        
           implementation libs.springboot.autoconfigure 
        
           testImplementation libs.grails.testing.support.core 
        
           testImplementation libs.servlet.api 
        
           testImplementation libs.spock.core 
        
           integrationTestImplementation libs.groovy.xml 
        
           integrationTestImplementation libs.greenmail 
        
           integrationTestImplementation libs.spring.web 
        
           integrationTestRuntimeOnly libs.grails.testing.support.web 
        
           integrationTestRuntimeOnly libs.slf4j.nop // Get rid of warning about missing slf4j implementation during test task 
        
           integrationTestRuntimeOnly libs.springboot.starter.tomcat

grails-mail/gradle/libs.versions.toml

Lines 15 to 36 in c0a82ec

    
           grails-core = { module = 'org.grails:grails-core', version.ref = 'grails' } 
        
           grails-gsp = { module = 'org.grails:grails-gsp', version.ref = 'gsp' } 
        
           grails-testing-support-core = { module = 'org.grails:grails-testing-support', version.ref = 'grails-testing-support' } 
        
           grails-testing-support-web = { module = 'org.grails:grails-web-testing-support', version.ref = 'grails-testing-support' } 
        
           grails-web-common = { module = 'org.grails:grails-web-common', version.ref = 'grails' } 
        
           grails-web-urlmappings = { module = 'org.grails:grails-web-url-mappings', version.ref = 'grails' } 
        
           greenmail = { module = 'com.icegreen:greenmail', version.ref = 'greenmail' } 
        
           groovy-core = { module = 'org.apache.groovy:groovy', version.ref = 'groovy' } 
        
           grails-docs = { module = 'org.grails:grails-docs', version.ref = 'grails' } 
        
           groovy-templates = { module = 'org.apache.groovy:groovy-templates', version.ref = 'groovy' } 
        
           groovy-xml = { module = 'org.apache.groovy:groovy-xml', version.ref = 'groovy' } 
        
           servlet-api = { module = 'jakarta.servlet:jakarta.servlet-api', version.ref = 'servlet-api' } 
        
           javamail-api = { module = 'jakarta.mail:jakarta.mail-api', version.ref = 'javamail' } 
        
           javamail-impl = { module = 'com.sun.mail:jakarta.mail', version.ref = 'javamail' } 
        
           slf4j-nop = { module = 'org.slf4j:slf4j-nop', version.ref = 'slf4j' } 
        
           spring-beans = { module = 'org.springframework:spring-beans', version.ref = 'spring' } 
        
           spring-context = { module = 'org.springframework:spring-context', version.ref = 'spring' } 
        
           spring-context-support = { module = 'org.springframework:spring-context-support', version.ref = 'spring' } 
        
           spring-web = { module = 'org.springframework:spring-web', version.ref = 'spring' } 
        
           springboot-autoconfigure = { module = 'org.springframework.boot:spring-boot-autoconfigure', version.ref = 'springboot' } 
        
           springboot-starter-tomcat = { module = 'org.springframework.boot:spring-boot-starter-tomcat', version.ref = 'springboot' } 
        
           spock-core = { module = 'org.spockframework:spock-core', version.ref = 'spock' }

now that we use a bom, you could completely delete libs.versions.toml and not add any additional lines of code.
That equals 36 less lines of maintenance.

It's way too verbose. I am surprised there aren't semi-colons required on every line as well. 🤣

What makes more sense and is easier to understand?

These 132 characters:

grails = '7.0.0-SNAPSHOT' 
grails-core = { module = 'org.grails:grails-core', version.ref = 'grails' } 
compileOnly libs.grails.core

or these 36

compileOnly 'org.grails:grails-core'

Objectively I see 75% less characters and a 1 less file to maintain.

matrei · 2024-10-23T07:29:37Z

First of all, you are quoting an old version before the new grails-bom was in effect.

Second, this is indeed DRY because the coords and versions are never repeated but can be and are used in multiple places.

codeconsole · 2024-10-23T07:44:15Z

DRY?

grails-core = { module = 'org.grails:grails-core' }
compileOnly libs.grails.core

compileOnly 'org.grails:grails-core'

You shouldn't have open another file to look up coordinates, but this is just my opinion. Hopefully it will evolve into something better someday...

Anyhow, I prefer SCOTTL format 😄

matrei · 2024-10-23T07:52:48Z

You shouldn't have open another file to look up coordinates

Why do you need to see the coordinates? If you do, ctrl-click in Intellij to go right to the declaration.

DRY?

compileOnly 'jakarta.servlet:jakarta.servlet-api' // Provided by the servlet container
testImplementation 'jakarta.servlet:jakarta.servlet-api'

codeconsole · 2024-10-24T07:09:13Z

DRY?

compileOnly 'jakarta.servlet:jakarta.servlet-api' // Provided by the servlet container
testImplementation 'jakarta.servlet:jakarta.servlet-api'

compared to? how is that handled any better with toml?

I suppose if that happened a lot, I would just create another dependency scope 😉

matrei · 2024-10-24T07:33:36Z

It appears we have different interpretations of DRY. My emphasis is on data and logic, whereas I perceive from this discussion that your focus is on the number of characters.

Another advantage of using a version catalog that I mentioned is the centralized location. grails-mail is a pretty small single-project build but it still has dependency declarations in two build files. By using a version catalog, it's easy to see and manage all dependencies used in the project. Intellij will also help with flagging unused dependencies in the toml file.

codeconsole · 2024-10-24T21:09:53Z

compileOnly 'jakarta.servlet:jakarta.servlet-api' // Provided by the servlet container
testImplementation 'jakarta.servlet:jakarta.servlet-api'

Can you explain your example? how does toml handle that any better?

My version of DRY is not characters. It is avoiding repeating yourself, which in my opinion, toml is not avoiding when used with a bom.

I also prefer clear and concise which it is lacking in both. It just further complicates the build and adds an extra learning curve to an already over complicated build process.

Do you honestly think it does not add an unnecessary additional learning curve to the majority of gradle users?

gradle/libs.versions.toml

matrei · 2024-10-25T05:48:55Z

Can you explain your example? how does toml handle that any better?

The example is the alternative version, when not using a version catalog. As 'jakarta.servlet:jakarta.servlet-api is needed in both compileOnly and testImplementation, the maven coordinates must declared in two places. With a version catalog, the coordinates are only declared once.

My version of DRY is not characters. It is avoiding repeating yourself, which in my opinion, toml is not avoiding when used with a bom.

From https://en.wikipedia.org/wiki/Don%27t_repeat_yourself:

The DRY principle is stated as "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system".

Do you honestly think it does not add an unnecessary additional learning curve to the majority of gradle users?

As I said earlier, I think version catalogs have several advantages. Gradle changes, hopefully to the better, for every version it releases. We should embrace those improvements, and I think version catalogs are not a very hard concept to grasp. If you are a totally new Gradle user, maybe it is even easier? Also with modern IDEs, you get auto-completion, click-through, unused code highlighting etc.

My subjective view is also that it is easier to read good named aliases, rather than parsing maven coordinate/version "gibberish" when I scan build files.

codeconsole · 2024-10-25T06:23:21Z

My subjective view is also that it is easier to read good named aliases, rather than parsing maven coordinate/version "gibberish" when I scan build files.

compileOnly 'jakarta.servlet:jakarta.servlet-api' 
testImplementation 'jakarta.servlet:jakarta.servlet-api'

vs 2 files for:

servlet-api = { module = 'jakarta.servlet:jakarta.servlet-api' }

compileOnly libs.servlet.api 
testImplementation libs.servlet.api

I honestly don't see the win here. Using toml you are still repeating yourself, and making it more difficult to follow. Not everyone uses IntelliJ and IntelliJ click through doesn't always work. The Grails IntelliJ plugin was broken for a year on Grails 6 and you couldn't click through on anything you used to be able to click on. In the end, the coordinates are the coordinates. Assigning a variable to them in a proprietary experimental format that has still yet to stabilize and provide a full feature set just seems like an over complication.

It's basically the route Grails 6 took over 5. It overcomplicated everything adding an unnecessary buildSrc folder and a bunch of configuration in settings.gradle. It just made things more difficult to follow and confused a lot of people. There are a lot of people thoroughly confused by this type of versioning as well.

At the end of the day you have TEN build files (.gradle +.toml) for a very simple plugin which unfortunately will cause a lot of people to get lost who would like to contribute. Why are we making things so complicated? Convention over configuration and DRY are paradigms to make things simpler, not more complicated. In my opinion, this plugin should consist of 1 build.gradle and that is it.

We will have to agree to disagree here. I am fine with the merge. I think you did a good job cleaning it up further. I just like to keep things as simple as possible and look for ways to remove configuration, not add it. For now, this is simpler than the previous version so let's just merge this and move on.

matrei · 2024-10-25T10:14:57Z

Agreed 😉

While the benefits in this project may seem marginal, I believe they become more apparent when these concepts are applied to more complex projects.

matrei added 6 commits October 17, 2024 10:29

test: Add log output to tests

143a3b1

fix(deps): Use grails-bom versions where available

1abcfc2

chore: Improve comment in documentation-config.gradle

6e5c6ff

test: Inject JavaMailSender to work with Spring 6

24bc5de

When creating the `JavaMailSender` via constructor, it could not connect to the greenmail server.

chore: Cleanup MailServiceSpec

11391f0

Merge branch '5.0.x' into matrei/use-bom

690bde5

matrei requested a review from codeconsole October 17, 2024 12:22

sbglasius reviewed Oct 17, 2024

View reviewed changes

build.gradle Show resolved Hide resolved

build.gradle Outdated Show resolved Hide resolved

matrei added 4 commits October 17, 2024 21:05

fix(deps): Put all dependencies in version catalog

6188e6d

fix(deps): Use more versions from grails-bom

5b56225

`groovy-templates` and `grails-docs` are also in `grails-bom`

fix(deps): Rename grails-gsp version catalog entry

5a03501

fix(deps): Set correct maven coords for grails-gsp

93ccd16

codeconsole requested changes Oct 18, 2024

View reviewed changes

gradle/libs.versions.toml Outdated Show resolved Hide resolved

matrei added 3 commits October 22, 2024 21:36

fix(deps): Rename the aliases for build dependencies

de2932c

To restrict the use of groups.

fix(deps): Use all available versions from grails-bom

bc2cea3

- Remove explicit versions to where versions are available in `grails-bom` - Use `org.eclipse.angus:jakarta.mail` instead of `com.sun.mail:jakarta.mail` - Restrict the use of version catalog alias groups

fix(deps): Update com.icegreen:greenmail

f502fc0

`2.0.1` -> `2.1.0`

matrei requested review from sbglasius and codeconsole October 23, 2024 04:57

codeconsole approved these changes Oct 24, 2024

View reviewed changes

gradle/libs.versions.toml Outdated Show resolved Hide resolved

matrei merged commit 97b67b8 into 5.0.x Oct 25, 2024
4 checks passed

matrei deleted the matrei/use-bom branch October 25, 2024 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dependency versions from the `grails-bom` where available #57

Use dependency versions from the `grails-bom` where available #57

matrei commented Oct 17, 2024

matrei commented Oct 17, 2024

sbglasius left a comment

codeconsole commented Oct 23, 2024

matrei commented Oct 23, 2024

codeconsole commented Oct 23, 2024 •

edited

Loading

matrei commented Oct 23, 2024

codeconsole commented Oct 23, 2024 •

edited

Loading

matrei commented Oct 23, 2024

codeconsole commented Oct 24, 2024

matrei commented Oct 24, 2024

codeconsole commented Oct 24, 2024 •

edited

Loading

matrei commented Oct 25, 2024

codeconsole commented Oct 25, 2024 •

edited

Loading

matrei commented Oct 25, 2024

Use dependency versions from the grails-bom where available #57

Use dependency versions from the grails-bom where available #57

Conversation

matrei commented Oct 17, 2024

matrei commented Oct 17, 2024

sbglasius left a comment

Choose a reason for hiding this comment

codeconsole commented Oct 23, 2024

matrei commented Oct 23, 2024

codeconsole commented Oct 23, 2024 • edited Loading

matrei commented Oct 23, 2024

codeconsole commented Oct 23, 2024 • edited Loading

matrei commented Oct 23, 2024

codeconsole commented Oct 24, 2024

matrei commented Oct 24, 2024

codeconsole commented Oct 24, 2024 • edited Loading

matrei commented Oct 25, 2024

codeconsole commented Oct 25, 2024 • edited Loading

matrei commented Oct 25, 2024

Use dependency versions from the `grails-bom` where available #57

Use dependency versions from the `grails-bom` where available #57

codeconsole commented Oct 23, 2024 •

edited

Loading

codeconsole commented Oct 23, 2024 •

edited

Loading

codeconsole commented Oct 24, 2024 •

edited

Loading

codeconsole commented Oct 25, 2024 •

edited

Loading