Preserving Parameter Names#15382
Conversation
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
We maintain 2 app generation tools currently - both grails-profile based and grails-forge based. Can you please update the associated profile skeleton too?
|
@jdaugherty - Thankyou for pointing this out! (TIL). I have added the same in |
|
@jamesfredley Conflicts are now resolved, could you please run the pipeline again. Thanks! |
|
If this is required I think we should default to it in GrailsGradlePlugin and provide an option on the extension to turn it off. Let's try to keep the build.gradle file clean. |
|
The compile plugin is for our project while the GrailsGradlePlugin is for all grails projects. |
|
@jdaugherty I have used the compiler plugin as reference, changes are made in grails-gradle-plugins.
|
rahulshishodia
force-pushed
the
preserve-param-names
branch
from
5902394 to
f888201
Compare
|
@jdaugherty can you please take a look again |
|
Assuming the tests pass, I'm good to merge this. @matrei Are you ok to merge this as well? I intend to update the grails docs separately from this PR. |
Added license information to GrailsGradlePreserveParametersSpec.
|
@jdaugherty I have added missing licence header, can you please re-trigger the workflow. Thanks! |
✅ All tests passed ✅🏷️ Commit: a565e5c Learn more about TestLens at testlens.app. |
| /** | ||
| * Keep class file parameter names so autowire by name can be supported without additional annotations such as '@Qualifier'. | ||
| */ | ||
| Property<Boolean> preserveParameterNames |
| } | ||
|
|
||
| /** | ||
| * Keep class file parameter names so autowire by name can be supported without additional annotations such as '@Qualifier'. |
There was a problem hiding this comment.
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.
| project.afterEvaluate { | ||
| boolean indyEnabled = grailsExtension?.indy?.getOrElse(false) ?: false | ||
| boolean indyEnabled = grailsExtension.indy?.getOrElse(false) ?: false | ||
| Boolean preserveParameterNames = grailsExtension.preserveParameterNames?.getOrNull() |
There was a problem hiding this comment.
- No need for null guard on
grailsExtension.preserveParameterNames, it can never benullas it is constructed in theGrailsExtensionconstructor. (Same forgrailsExtension.indyon the previous line).
There was a problem hiding this comment.
it can be null if someone sets a provider- which says do not configure. this needs to stay
There was a problem hiding this comment.
@jdaugherty I tested, and it will still not be null with a provider { null }.
| project.tasks.withType(GroovyCompile).configureEach { GroovyCompile c -> | ||
| c.groovyOptions.optimizationOptions.indy = indyEnabled | ||
|
|
||
| if (preserveParameterNames != null) { |
There was a problem hiding this comment.
This can never be null as its convention is true. If explicitly set to null in the extension configuration, it will still resolve to true as that is its convention setting.
There was a problem hiding this comment.
Yes it can, a user can set a provider that returns null
There was a problem hiding this comment.
Ah, thank you! Can we create a test showing this in action?
| result.output.contains("HAS_PRESERVE_PARAM_ENABLED=false") | ||
| } | ||
|
|
||
| def "preserveParameterNames is set to true when configured as explicit null"() { |
There was a problem hiding this comment.
Can we be more descriptive as to why null => true? (fallback to the convention)
4 participants
Reference:
org.apache.grails.buildsrc.CompilePlugin#configureCompilerIssue: #13028