Skip to content

Build restructuring#9

Merged
jdaugherty merged 17 commits into
feedbackfrom
more-feedback
Feb 24, 2026
Merged

Build restructuring#9
jdaugherty merged 17 commits into
feedbackfrom
more-feedback

Conversation

@matrei
Copy link
Copy Markdown
Contributor

@matrei matrei commented Feb 23, 2026

No description provided.

Comment thread examples/app1/build.gradle
Comment thread plugin/build.gradle
Copy link
Copy Markdown
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

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

Here's what I like:

  1. general gradle clean up & consistency
  2. removal of the project specific name so it can be generic (@sbglasius feedback)
  3. using the type specific extensions like AssetPipelineExtension
  4. using withPlugin() to reduce configuration

Here's what I think we can improve:

  1. it appears the publish plugin works when using the convention plugin approach so we don't have to define this in the root - https://github.com/grails-plugins/grails-server-timing/tree/publish-changes

What I don't like:

  1. I think config.app-debug is too generic. It happens to only configure debug now, but in my other projects, I also configure profiler settings, common bootRun jvm args, and even a runJar task to run like production would in a container. I'd rather we keep this more generically named to encourage us to centralize all of the BootRun configuration in one spot.

@jdaugherty
Copy link
Copy Markdown
Contributor

As TODO, once we merge, we still need to update the documentation (which I'm happy to do). The only reason I added it to this project was to make it easier to find stuff. Maybe it belongs outside of this project (the project structure stuff)

@matrei matrei requested a review from jdaugherty February 24, 2026 08:35
assetDevelopmentRuntime 'org.webjars.npm:bootstrap'
assetDevelopmentRuntime 'org.webjars.npm:bootstrap-icons'
assetDevelopmentRuntime 'org.webjars.npm:jquery'
add('assetDevelopmentRuntime', 'org.webjars.npm:bootstrap')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this because intellij didn't detect the scope? With the asset plugin defined in the same file, this seems unnecessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the reason. I can revert it if you like it better the DSL way.

Copy link
Copy Markdown
Contributor

@jdaugherty jdaugherty Feb 24, 2026

Choose a reason for hiding this comment

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

I'm just worried about consistency. It's a shame that IntelliJ seems to always half implements this stuff. What are other people's thoughts? @jamesfredley @sbglasius

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm going to go ahead and merge this, if @jamesfredley or @sbglasius has any feedback on this we can address later.

Comment thread code-coverage/build.gradle Outdated
Comment thread plugin/build.gradle
Comment thread settings.gradle
Comment thread build-logic/src/main/groovy/config.publish-root.gradle
Comment thread plugin/build.gradle Outdated
Comment thread .skills/gradle-best-practices.md
@jdaugherty jdaugherty merged commit fbabf0f into feedback Feb 24, 2026
5 checks passed
@jdaugherty jdaugherty deleted the more-feedback branch February 24, 2026 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants