-
Notifications
You must be signed in to change notification settings - Fork 405
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
DGPv2: fix noJdkLink/noStdlibLink/noAndroidSdkLink #3832
DGPv2: fix noJdkLink/noStdlibLink/noAndroidSdkLink #3832
Conversation
* | ||
* @see enableKotlinStdLibDocumentationLink | ||
*/ | ||
@Deprecated("Replaced with to enableKotlinStdLibDocumentationLink. The value must be inverted.", level = ERROR) |
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.
why deprecation level error? to force migration to enableKotlinStdLibDocumentationLink
API?
Or it is technically difficult to map inverted value of noStdlibLink
to enableKotlinStdLibDocumentationLink
and back?
For migration purposes I'd try to stick as much as possible to warnings first.
If I understand correctly, Dokka v1 has that declaration noStdlibLink: Property<Boolean>
.
I was thinking about creating something like this:
val deprecatedInverseProperty = objects
.property(Boolean::class.java)
val newUserFacingProperty = objects
.property(Boolean::class.java)
.convention(true)
fun <T: Any, D: Any> Property<T>.getValueProviderWithDeprecatedProperty(
deprecatedProperty: Property<D>,
conventionValue: T,
mapFromDeprecatedToNew: (D) -> T,
reportContradiction: (Property<T>, Property<D>) -> Nothing
): Provider<T> {
if (!deprecatedProperty.isPresent) return this
val mappedDeprecatedValue = deprecatedProperty.map { mapFromDeprecatedToNew(it) }
if (!isPresent) return mappedDeprecatedValue
val newValue = this.get()
val deprecatedValue = mappedDeprecatedValue.get()
return when {
newValue == conventionValue && deprecatedValue == conventionValue -> this
newValue == conventionValue && deprecatedValue != conventionValue -> mappedDeprecatedValue
newValue != conventionValue && deprecatedValue == conventionValue -> this
newValue != conventionValue && deprecatedValue == newValue -> this
newValue != conventionValue && deprecatedValue != newValue -> reportContradiction(this, deprecatedProperty)
else -> throw IllegalStateException()
}
}
But I guess this is too heavy.
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.
Those properties are not very popular and are really needed only in small amount of use-cases.
So I think it's fine to deprecate them with ERROR right away
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.
why deprecation level error?
I decided to make the deprecation level ERROR
because these DGPv2 is an opt-in and when users enable v2 some migration is expected.
to force migration to
enableKotlinStdLibDocumentationLink
API?
Yes, when v2 is enabled these properties are completely non-functional without migration.
- Or it is technically difficult to map inverted value of
noStdlibLink
toenableKotlinStdLibDocumentationLink
and back?
Yes.
I like the idea of a workaround, but I don't want to spend the rest of the week writing tests for it :)
I think a property transformer like that would be the responsibility of Gradle to add (maybe gradle/gradle#27315?).
* | ||
* @see enableKotlinStdLibDocumentationLink | ||
*/ | ||
@Deprecated("Replaced with to enableKotlinStdLibDocumentationLink. The value must be inverted.", level = ERROR) |
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.
Those properties are not very popular and are really needed only in small amount of use-cases.
So I think it's fine to deprecate them with ERROR right away
The last PR had mistakes 🤦
enableKotlinStdLibDocumentationLink
is not a direct replacement fornoStdlibLink
, because the value must be inverted.@Deprecate
fornoJdkLink
andnoAndroidSdkLink
were the wrong way around.I decided to make the deprecation level ERROR because these DGPv2 is an opt-in and migration is expected. Additionally, these properties are completely non-functional without migration.
#2515
KT-71758