-
-
Notifications
You must be signed in to change notification settings - Fork 968
Preserving Parameter Names #15382
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
base: 8.0.x
Are you sure you want to change the base?
Preserving Parameter Names #15382
Changes from all commits
38e1ed7
127f01e
13675ab
7661d1c
1a83ae0
88586a0
8ec535d
aa0afee
f231d92
90b8a5a
fc9a55a
f888201
fa61946
de806d8
f531f61
7032937
a4f2739
7d9e779
95321a0
91ee3c2
1c3a1e4
7a1a0f0
a565e5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ class GrailsExtension { | |
| this.project = project | ||
| this.pluginDefiner = new PluginDefiner(project) | ||
| this.indy = project.objects.property(Boolean).convention(false) | ||
| this.preserveParameterNames = project.objects.property(Boolean).convention(true) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -106,6 +107,11 @@ class GrailsExtension { | |
| this.indy.set(enabled) | ||
| } | ||
|
|
||
| /** | ||
| * Keep class file parameter names so autowire by name can be supported without additional annotations such as '@Qualifier'. | ||
| */ | ||
| Property<Boolean> preserveParameterNames | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be |
||
|
|
||
| DependencyHandler getPlugins() { | ||
| if (pluginDefiner == null) { | ||
| pluginDefiner = new PluginDefiner(project) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,10 +227,17 @@ class GrailsGradlePlugin implements Plugin<Project> { | |
| // Configure indy and log status after evaluation so user's grails { } block has been applied | ||
| GrailsExtension grailsExtension = project.extensions.findByType(GrailsExtension) | ||
| project.afterEvaluate { | ||
| boolean indyEnabled = grailsExtension?.indy?.getOrElse(false) ?: false | ||
| boolean indyEnabled = grailsExtension.indy?.getOrElse(false) ?: false | ||
| Boolean preserveParameterNames = grailsExtension.preserveParameterNames?.getOrNull() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it can be null if someone sets a provider- which says do not configure. this needs to stay
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jdaugherty I tested, and it will still not be null with a |
||
|
|
||
| project.tasks.withType(GroovyCompile).configureEach { GroovyCompile c -> | ||
| c.groovyOptions.optimizationOptions.indy = indyEnabled | ||
|
|
||
| if (preserveParameterNames != null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can never be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it can, a user can set a provider that returns null
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thank you! Can we create a test showing this in action? |
||
| c.groovyOptions.parameters = preserveParameterNames | ||
| } | ||
| } | ||
|
|
||
| if (!indyEnabled) { | ||
| project.logger.info('Grails: Groovy invokedynamic (indy) is disabled to improve performance (see issue #15293).') | ||
| project.logger.info(' To enable invokedynamic: grails { indy = true } in build.gradle') | ||
|
|
@@ -520,7 +527,7 @@ ${importStatements} | |
|
|
||
| protected GrailsExtension registerGrailsExtension(Project project) { | ||
| if (project.extensions.findByName('grails') == null) { | ||
| project.extensions.add('grails', new GrailsExtension(project)) | ||
| project.extensions.create('grails', GrailsExtension, project) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.grails.gradle.plugin.core | ||
|
|
||
| class GrailsGradlePreserveParametersSpec extends GradleSpecification { | ||
|
|
||
| def "Grails extension is created with default preserveParameterNames = true"() { | ||
| given: | ||
| setupTestResourceProject('preserve-params-default') | ||
|
|
||
| when: | ||
| def result = executeTask('inspectPreserveParam') | ||
|
|
||
| then: | ||
| result.output.contains("HAS_PRESERVE_PARAM_ENABLED=true") | ||
| } | ||
|
|
||
| def "preserveParameterNames can be configured to false via grails block"() { | ||
| given: | ||
| setupTestResourceProject('preserve-params-disabled') | ||
|
|
||
| when: | ||
| def result = executeTask('inspectPreserveParam') | ||
|
|
||
| then: | ||
| result.output.contains("HAS_PRESERVE_PARAM_ENABLED=false") | ||
| } | ||
|
|
||
| def "preserveParameterNames is set to true when configured as explicit null"() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we be more descriptive as to why |
||
| given: | ||
| setupTestResourceProject('preserve-params-null') | ||
|
|
||
| when: | ||
| def result = executeTask('inspectPreserveParam') | ||
|
|
||
| then: | ||
| result.output.contains("HAS_PRESERVE_PARAM_ENABLED=true") | ||
| } | ||
|
|
||
| def "GroovyCompile tasks get parameters = true when preserveParameterNames is enabled"() { | ||
| given: | ||
| setupTestResourceProject('preserve-params-enabled') | ||
|
|
||
| when: | ||
| def result = executeTask('inspectPreserveParam') | ||
|
|
||
| then: | ||
| result.output.contains("HAS_PRESERVE_PARAM_ENABLED=true") | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| plugins { | ||
| id 'org.apache.grails.gradle.grails-app' | ||
| } | ||
|
|
||
| tasks.register('inspectPreserveParam') { | ||
| doLast { | ||
| def compileTasks = tasks.withType(GroovyCompile) | ||
| def paramsEnabled = compileTasks.every { it.groovyOptions.parameters } | ||
| println "HAS_PRESERVE_PARAM_ENABLED=${paramsEnabled}" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| grailsVersion=__PROJECT_VERSION__ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| rootProject.name = 'test-preserve-params-default' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| plugins { | ||
| id 'org.apache.grails.gradle.grails-app' | ||
| } | ||
|
|
||
| grails { | ||
| preserveParameterNames = false | ||
| } | ||
|
|
||
| tasks.register('inspectPreserveParam') { | ||
| doLast { | ||
| def compileTasks = tasks.withType(GroovyCompile) | ||
| def paramsEnabled = compileTasks.every { it.groovyOptions.parameters } | ||
| println "HAS_PRESERVE_PARAM_ENABLED=${paramsEnabled}" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| grailsVersion=__PROJECT_VERSION__ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| rootProject.name = 'test-preserve-params-disabled' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| plugins { | ||
| id 'org.apache.grails.gradle.grails-app' | ||
| } | ||
|
|
||
| grails { | ||
| preserveParameterNames = true | ||
| } | ||
|
|
||
| tasks.register('inspectPreserveParam') { | ||
| doLast { | ||
| def compileTasks = tasks.withType(GroovyCompile) | ||
| def paramsEnabled = compileTasks.every { it.groovyOptions.parameters } | ||
| println "HAS_PRESERVE_PARAM_ENABLED=${paramsEnabled}" | ||
|
jdaugherty marked this conversation as resolved.
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| grailsVersion=__PROJECT_VERSION__ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| rootProject.name = 'test-preserve-params-enabled' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| plugins { | ||
| id 'org.apache.grails.gradle.grails-app' | ||
| } | ||
|
|
||
| grails { | ||
| preserveParameterNames = null | ||
| } | ||
|
|
||
| tasks.register('inspectPreserveParam') { | ||
| doLast { | ||
| def compileTasks = tasks.withType(GroovyCompile) | ||
| def paramsEnabled = compileTasks.every { it.groovyOptions.parameters } | ||
| println "HAS_PRESERVE_PARAM_ENABLED=${paramsEnabled}" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| grailsVersion=__PROJECT_VERSION__ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| rootProject.name = 'test-preserve-params-null' |
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.
Suggestion (and keep line length under 120 for readability):
Keep method and constructor parameter names in class files, allowing frameworks such as Spring to use parameter names for dependency resolution, including autowiring by name without requiring annotations such as @Qualifier.