-
Notifications
You must be signed in to change notification settings - Fork 5k
Properties of extensions that are containers should have Kotlin DSL accessors for existing elements #23665
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
Conversation
.../integTest/kotlin/org/gradle/kotlin/dsl/integration/ProjectSchemaAccessorsIntegrationTest.kt
Show resolved
Hide resolved
...rojects/model-core/src/main/java/org/gradle/internal/reflect/JavaPropertyReflectionUtil.java
Show resolved
Hide resolved
subprojects/reporting/src/main/java/org/gradle/api/reporting/ReportingExtension.java
Outdated
Show resolved
Hide resolved
...cts/testing-base/src/main/java/org/gradle/testing/base/internal/DefaultTestingExtension.java
Outdated
Show resolved
Hide resolved
f6d195d
to
fb0bfc8
Compare
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.
This is going into the right direction.
I requested a few changes.
This also raises questions we need to discuss.
...cts/core/src/main/java/org/gradle/api/internal/AbstractPolymorphicDomainObjectContainer.java
Outdated
Show resolved
Hide resolved
...cts/testing-base/src/main/java/org/gradle/testing/base/internal/DefaultTestingExtension.java
Outdated
Show resolved
Hide resolved
subprojects/testing-base/src/main/java/org/gradle/testing/base/TestingExtension.java
Outdated
Show resolved
Hide resolved
subprojects/reporting/src/main/java/org/gradle/api/reporting/ReportingExtension.java
Outdated
Show resolved
Hide resolved
subprojects/reporting/src/main/java/org/gradle/api/reporting/ReportingExtension.java
Outdated
Show resolved
Hide resolved
subprojects/kotlin-dsl/src/main/kotlin/org/gradle/kotlin/dsl/accessors/ProjectSchemaProvider.kt
Outdated
Show resolved
Hide resolved
...ugins/src/main/kotlin/org/gradle/kotlin/dsl/provider/plugins/DefaultProjectSchemaProvider.kt
Outdated
Show resolved
Hide resolved
...ugins/src/main/kotlin/org/gradle/kotlin/dsl/provider/plugins/DefaultProjectSchemaProvider.kt
Outdated
Show resolved
Hide resolved
...e/src/test/groovy/org/gradle/api/internal/DefaultPolymorphicDomainObjectContainerTest.groovy
Outdated
Show resolved
Hide resolved
eae96f7
to
bc55efa
Compare
Do a bit of cleanup in tests
The reason behind is change is to allow for the generation of Kotlin accessors for existing elements in this type of container.
bc55efa
to
47b182d
Compare
TestSuiteContainer is the perfect name for the new return type we introduced for TestingExtension.getSuites(). However there is already a class named like that. Fortunately it's an obsolete class, so we can rename it and reuse its original name.
Since these entries define the signature of generated Kotlin accessors, there is no point in allowing duplicates for them. They would just generate the same thing. So we are changing the collections we use for storing them to Sets.
47b182d
to
84ec371
Compare
subprojects/plugins/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java
Outdated
Show resolved
Hide resolved
and prefer @PublicType over HasPublicType for existing conventions, source sets and containers
by unifying type casts by calling existing methods instead of repeating code by adding javadoc
fixing parameter name references and typos in javadoc and making checkstyle happy
@bot-gradle test this |
I've triggered the following builds for you: |
subprojects/testing-base/src/main/java/org/gradle/testing/base/TestSuiteSpecContainer.java
Outdated
Show resolved
Hide resolved
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.
A few requests, mostly about documentation:
- Name the container type for test suites without breaking change, see the discussion above
- Update
TestSuitesKotlinDSLDependenciesIntegrationTest
to use the new accessors when possible - Update documentation snippets and samples to use the new accessors when possible, e.g. in https://docs.gradle.org/current/userguide/jvm_test_suite_plugin.html or in https://docs.gradle.org/current/samples/sample_jvm_multi_project_with_code_coverage_distribution.html (there are others)
- Update the documentation on when type-safe accessors are available at https://docs.gradle.org/current/userguide/kotlin_dsl.html#kotdsl:accessor_applicability
- Add the new feature to release notes with the test suites as an example snippet and linking to the documentation above
@bot-gradle test this |
I've triggered the following builds for you: |
subprojects/core-api/src/main/java/org/gradle/api/reflect/PublicType.java
Outdated
Show resolved
Hide resolved
subprojects/testing-base/src/main/java/org/gradle/testing/base/TestSuiteSpecContainer.java
Outdated
Show resolved
Hide resolved
We don't want to incur the potential breaking changes that can result from renaming the original TestSuiteContainer. We will instead use a worse, but free name: TestSuites
2d677ae
to
ad39a6d
Compare
This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please ask the team to reopen the PR or push a new PR. Thank you for your contributions. |
Hey, I got the same issue mentioned in the ticket and I am curious what's the status of this PR, because all GitHub checks did run successfully (I can't see the expired TC build). |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. |
This pull request has been automatically closed due to inactivity. |
This PR makes the Kotlin DSL generate accessors for elements of properties of extensions that are containers.
For example this makes the DSL for JVM test suites more discoverable and terse.
Before:
After:
To support use cases that are valuable for this work such as JVM test suites and Android build types, the polymorphic containers now honor the declared public types of their elements in their schema. They do so in a way that the schema is stable whichever is the state of their elements, realised or not: the base element type of container elements is preferred over their concrete type.
To support this a new
@PublicType
annotation is introduced to statically declare preferred public types of DSL objects. This will replace the oldHasPublicType
interface that requires realised instances causing changing collection schemas when elements get realised.This PR doesn't change the accessors exposed for tasks that expose more concrete types than other polymorphic containers, it also doesn't change how its schema is unstable after realising its elements. This should be fixed eventually but is out of scope of this PR.