Java interop version of toOptional()

[Egorand]

We're sometimes calling toOptional() from Java and it would be more convenient to have Optional.toOptional() instead of OptionalKt.toOptional().

[bensandee]

I'm just a consumer of the API (private repo, would be affected by the breaking change but not in a way I couldn't recover from in 5 minutes), but it seems that the argument can be distilled into two things:

• go ahead and break it, not enough people use it and we don't want to have a secondary artifact/package (potentially with version numbers).
• don't break things, follow semantic versioning, it's the principle of the thing.

Let me ask a rhetorical question -- when driving, do you use a turn signal when you think nobody is looking? IMO, it's best to do the "right thing" even when nobody's looking.

[arturdryomov]

@tbsandee, I just got some serious 13 Reasons Why vibe.

Still not convinced that renaming can be considered a behavior change though. I’m kind of in, but still consider this case an extreme over pragmatism at its best.

The worst thing I can imagine right now is related to an argument from Artem about Optional being an exchange class between components.

• We have a library A 1.0.0 and library B 1.0.0 exchanging data using Optional.
• A moves to 2.0.0 and com.gojuno.koptional2.
• B updates A to 2.0.0 and is forced to use com.gojuno.koptional2.

Should we provide an interop then? I need a drink.

[vanniktech]

I sure hope it's a good drink :D

[artem-zinnatullin]

We don't have to provide interop (although we can):

• Library is very small and interop can be done through going back to nullable types or by having really small conversion extension functions
• As pointed above, for most of the users conversion can be done in 100% automated "find and replace" manner and we can provide sed command in the changelog that'll do the migration

[artem-zinnatullin]

Soooo?

[artem-zinnatullin]

Can I go ahead and:

1. Merge this PR
2. Make a PR with groupId and packageName update
3. Release v2
4. Provide single cli command in release notes to update imports

?

Pros:

• We don't break people
• We ship new version with better class name for Java consumers
• We can sleep well knowing that we shipped new version and didn't break people

Cons:

• People will need to run command (or find + replace in IDE) instead of just version change in build config

I'd like to get this going.

[arturdryomov]

@artem-zinnatullin, I’m in. Still sounds ridiculous for such a small library, but we made a mistake not taking Java into account and we are gonna pay the price of having 2 in the package name. Let’s do it.

[artem-zinnatullin]

Kool, implementing then 👍

[artem-zinnatullin]

@Egorand there is a compilation error (you can see it on CI):

e: /opt/project/koptional/src/main/kotlin/com/gojuno/koptional/Optional.kt: (1, 1): Duplicate JVM class name 'com/gojuno/koptional/Optional' generated from: package-fragment com.gojuno.koptional, Optional
e: /opt/project/koptional/src/main/kotlin/com/gojuno/koptional/Optional.kt: (5, 1): Duplicate JVM class name 'com/gojuno/koptional/Optional' generated from: package-fragment com.gojuno.koptional, Optional

[artem-zinnatullin]

That'd be nice!

[artem-zinnatullin]

@Egorand can you please update PR to make it compile and add few Java test cases?

[Egorand]

Sure, will update the PR tomorrow

[Egorand]

I think I've actually discovered a better solution that won't break existing clients and will improve Java experience: adding a companion object with an extra toOptional() method. Kotlin clients should continue using the top-level extension function, Java clients would have to change from OptionalKt.toOptional() to Optional.toOptional() (the latter still works though). Also:

1. There's no way to fix compilation with @JvmName since we're ending up with two classes called Optional - one for the sealed class Optional and the other for the top-level function - that's a broken approach.
2. I added a test in Java, I used plain JUnit, let me know if you'd like me to make it in Spec for Java (never used it, is it even a thing?) or JUnit 5 or anything else.
3. I thought about documenting the two toOptional() version, but I see that there's no documentation at all in the file - should I add it?

[artem-zinnatullin]

1. Indeed, I don't see a way to solve this atm
2. Yep, looks good 👍
3. I have a stupid idea that seems to work (see below)

So, companion obj + @JvmStatic pretty much hides this 2nd toOptional() from Kotlin code, right.

What if we "hide" the first toOptional() from Java? #yolo

If you add @JvmName("_toOptionalKotlin") to the top-level toOptional() function, this will remove it from autocomplete on Java side

[Egorand]

That would be a breaking change, since Java clients who were using OptionalKt.toOptional() will have to use the new method name. I like the idea in general, and it would be good to implement it for the next major release, for now I think it's fine to document correct usage for Kotlin and Java.

[artem-zinnatullin]

Yeah, we already agreed on 2.x release though.

Would be great to make initial approach with single toOptional function declared properly for both Kotlin and Java work…

Technically it seems okay to have toOptional declared as a static function in Optional class, maybe we should submit a ticket to https://youtrack.jetbrains.com and describe our use case

Another solution is to write Koptional core in Java with proper Kotlin annotations and properly placed functions and see if Kotlin compiler works with that, I mean, that sounds like a fun experiment (not that I have time for it tho…)

[Egorand]

The problem is that extension functions declared inside classes or objects can only be used as extensions inside those classes/objects, hence you'll need to always do Optional.toOptional(), even in Kotlin. Rewriting the core in Java destroys the potential of making the library multi-platform. Opening a discussion on YouTrack sounds like a great idea, I'd love to get more insight on the issue.

[arturdryomov]

Let’s go ahead without hacks and do a 2.x as it was proposed in the first place. We screwed up with Java compatibility and it is OK to acknowledge it.

[arturdryomov]

So, actually, what do we agree on?

[Egorand]

As I mentioned, the change is not breaking, so it'd be fine as a minor update

[dmitry-novikov]

@ming13, @artem-zinnatullin, are you OK to merge it as minor update?

[artem-zinnatullin]

ok, I double checked current version of PR, I think I'm good to merge this as non breaking change 👍

Kotlin:

• Nothing changes, you still work with toOptional() extension most of the time
• Now you can also use Optional.toOptional() which is probably not that bad tbh (I didn't like that back then, but now I find it fine)

Java:

• We're still backwards compatible, extension is still there and you can keep calling OptionalKt.toOptional() (I didn't like it back then, but now I find it ok, library was not intended to be used from Java so it's fine)
• Now you can use Optional.toOptional() which is very good

@Egorand just couple things:

• Can you please add a Java test to verify that we keep compatibility with OptionalKt.toOptional()? I mean it works, but a test would be great to make sure we don't break it in future
• Can you please add ~java~kotlindoc to both functions and explain that OptionalKt one should not be used from Java and @JvmStatic one is preferred?
• You can add Java interop section to the README, or I can do it later, up2u
• Maybe we should duplicate the body of original toOptional() extension to avoid method call, some people are already upset about having to wrap their data with Optional, additional indirection might make their feelings worse, I don't think that's critical though, it should be easily inlined by JIT or AOT
• I'm personally really sorry it took us so long to figure out, wish things were easier :(

[Egorand]

Yeah no worries! I'll try to get the changes in today

[Egorand]

@artem-zinnatullin 0dafabb

[artem-zinnatullin]

Sorry for delay, had miscommunication with Artur and was waiting for his review while he was waiting for me to merge heh

[Egorand]

Sweet! Are you planning a release any time soon?

[artem-zinnatullin]

Yep, we can do a release with this PR only or wait for rest

[artem-zinnatullin]

@Egorand 1.4.0 was just released with this change, guys at Juno have some workload backpressure, I'm gonna take care of processes for some time