Skip to content

Conversation

jbartok
Copy link
Member

@jbartok jbartok commented Jan 26, 2023

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:

testing {
    suites {
        named<JvmTestSuite>("test") {
            useJUnit()
        }
    }
}

After:

testing {
    suites {
        test {
            useJUnit()
        }
    }
}


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 old HasPublicType 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.

@jbartok jbartok self-assigned this Jan 26, 2023
@jbartok jbartok added a:feature A new functionality in:kotlin-dsl labels Jan 26, 2023
@jbartok jbartok added this to the 8.1 RC1 milestone Jan 26, 2023
@jbartok jbartok force-pushed the jbartok/extra-accessors branch 2 times, most recently from f6d195d to fb0bfc8 Compare January 31, 2023 09:11
@jbartok jbartok marked this pull request as ready for review February 2, 2023 08:49
@jbartok jbartok requested a review from a team as a code owner February 2, 2023 08:49
@jbartok jbartok requested a review from tresat February 2, 2023 08:49
Copy link
Member

@eskatos eskatos left a 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.

@jbartok jbartok force-pushed the jbartok/extra-accessors branch 3 times, most recently from eae96f7 to bc55efa Compare February 8, 2023 07:21
@jbartok jbartok force-pushed the jbartok/extra-accessors branch from bc55efa to 47b182d Compare February 8, 2023 07:52
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.
@jbartok jbartok force-pushed the jbartok/extra-accessors branch from 47b182d to 84ec371 Compare February 8, 2023 10:47
@tresat tresat requested review from big-guy and removed request for tresat February 8, 2023 15:56
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
@eskatos eskatos requested a review from hythloda as a code owner February 13, 2023 09:31
@eskatos
Copy link
Member

eskatos commented Feb 13, 2023

@bot-gradle test this

@gradle gradle deleted a comment from jbartok Feb 13, 2023
@bot-gradle
Copy link
Collaborator

I've triggered the following builds for you:

Copy link
Member

@eskatos eskatos left a 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:

@eskatos
Copy link
Member

eskatos commented Feb 13, 2023

@bot-gradle test this

@gradle gradle deleted a comment from eskatos Feb 13, 2023
@bot-gradle
Copy link
Collaborator

I've triggered the following builds for you:

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
@jbartok jbartok force-pushed the jbartok/extra-accessors branch from 2d677ae to ad39a6d Compare February 14, 2023 13:13
@eskatos eskatos removed this from the 8.1 RC1 milestone Feb 21, 2023
@eskatos eskatos marked this pull request as draft February 21, 2023 10:25
@github-actions
Copy link
Contributor

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.

@hfhbd
Copy link
Contributor

hfhbd commented Dec 16, 2023

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).

Copy link
Contributor

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.

@github-actions github-actions bot added the stale label Jan 22, 2024
Copy link
Contributor

github-actions bot commented Feb 5, 2024

This pull request has been automatically closed due to inactivity.

@github-actions github-actions bot closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Properties of extensions that are containers should have Kotlin DSL accessors for existing elements

7 participants