Skip to content
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

Add FunSpec#jvmModifiers overload to fix chaining #653

Closed
wants to merge 1 commit into from

Conversation

ZacSweers
Copy link
Collaborator

Resolves #638

Copy link
Collaborator

@Egorand Egorand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution actually breaks compilation for Java users, since HIDDEN makes the old method inaccessible (I'm not entirely sure how this works, I just see the compilation error. Jake would know more). Besides, Java users would have to switch to jvmModifiersNew(), which is a pretty weird method name. Even though maintaining ergonomic Java API is not our top priority, I think this kind of change landing inside a minor version update is not good. I still think we should postpone this fix until 2.0. Luckily, in Kotlin it's very easy to wrap the method inside apply {}.

fun jvmModifiers(modifiers: Iterable<Modifier>) {
@JvmName("jvmModifiers")
@Deprecated(
message = "This API was missing the Builder return type and breaks chaining. " +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message here should target library users, not maintainers, something like "Deprecated. Use jvmModifiersNew()." would do.

replaceWith = ReplaceWith("jvmModifiers(modifiers)"),
level = HIDDEN
)
fun jvmModifersOld(modifiers: Iterable<Modifier>) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Modifers -> Modifiers

@ZacSweers
Copy link
Collaborator Author

Talked offline, will leave to a future 2.x

@ZacSweers ZacSweers closed this Mar 27, 2019
@ZacSweers ZacSweers deleted the z/jvmModifiers branch March 27, 2019 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants