-
Notifications
You must be signed in to change notification settings - Fork 31
misc: plannedRemoval annotation #1415
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
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
* Any code using this API should migrate to the suggested replacement as soon as possible. | ||
*/ | ||
@RequiresOptIn( | ||
message = "This API is deprecated and will be removed in an upcoming minor version.", |
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.
question: Can we use major
and minor
in this message to indicate which minor version the API will be removed in?
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.
If we could inherit from annotation classes, maybe, but Kotlin doesn't allow us to do so. Unless you know a workaround.
message = "This API is deprecated and will be removed in an upcoming minor version.", | ||
level = RequiresOptIn.Level.WARNING, | ||
) | ||
public annotation class DeprecatedUntilVersion( |
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.
DeprecatedUntilVersion
makes it sound like we'll be un-deprecating this API after X version. What if we called it DeprecatedPendingRemoval
?
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.
Yeah, I agree. When I first read our doc about the annotation, it reads like the annotation is meant to do that. I'd like for @ianbotsf to chime in. I personally like @UpcomingDeletion
, @WillBeRemoved
, or something similar. We shouldn't need to include anything about deprecation because the @Deprecated
annotation will also always be included.
This is a slightly bigger change than it seems by the way. I'll need to update the annotation regex and searches in a few places.
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.
Yeah, if we need to keep @Deprecated
then including ...Deprecated...
in the new annotation feels redundant. I like @PendingRemoval
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.
Agreed, we don't need use the word "deprecated" twice. I don't love "upcoming deletion" or "will be removed" because they sound like promises and might imply a definitive timeline to callers. "Pending removal" sounds better but I think I'd prefer @PlannedRemoval
.
message = "This API is deprecated and will be removed in an upcoming minor version.", | ||
level = RequiresOptIn.Level.WARNING, | ||
) | ||
public annotation class DeprecatedUntilVersion( |
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.
Is it possible for this to extend / implement Deprecated
so we don't need two annotations?
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.
Unless you know some workaround, we can't extend @Deprecated
. Kotlin doesn't allow us to do that. We can "implement" deprecated again but that doesn't seem ideal. It doesn't seem like we can fully implement the same functionality ourselves.
https://github.com/JetBrains/kotlin/blob/2.2.20/libraries/stdlib/src/kotlin/Annotations.kt#L36
message = "This API is deprecated and will be removed in an upcoming minor version.", | ||
level = RequiresOptIn.Level.WARNING, | ||
) | ||
public annotation class DeprecatedUntilVersion( |
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.
Agreed, we don't need use the word "deprecated" twice. I don't love "upcoming deletion" or "will be removed" because they sound like promises and might imply a definitive timeline to callers. "Pending removal" sounds better but I think I'd prefer @PlannedRemoval
.
* Any code using this API should migrate to the suggested replacement as soon as possible. | ||
*/ | ||
@RequiresOptIn( | ||
message = "This API is deprecated and will be removed in an upcoming minor version.", |
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.
nit: Hopefully this doesn't happen for a while, but we may make a major version bump that deletes APIs, so this message should probably just say "... scheduled for removal in an upcoming minor version"
This comment has been minimized.
This comment has been minimized.
Affected ArtifactsChanged in size
|
Issue #
N/A
Description of changes
Adds annotation to indicate when a deprecated API will be removed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.