Java interop version of toOptional() by Egorand · Pull Request #17 · gojuno/koptional

Egorand · 2018-03-15T19:45:42Z

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

artem-zinnatullin

I'm putting "Request changes" for now just to block merge until we have consensus on 2.x release, no actual changes in PR required :)

artem-zinnatullin · 2018-03-17T02:21:12Z

@@ -1,3 +1,5 @@
+@file:JvmName("Optional")


I 100% agree with this change, the only problem is that it's a breaking change for Java consumers (class name change) and we'll need to make a 2.x release for that

However I'm not opposed to that

@dmitry-novikov @ming13 @nostra13 are you ok with 2.x release?

If yes — we'll need separate issue and PRs for that (Group name will also need to be updated to com.gojuno.koptional2)

I'm OK with bumping major version to indicate breaking change, but against package change because is will affect a lot of consumers and most of them are Kotlin based. So I would better force Java consumers to refactor their codebase because of class name change =)

@artem-zinnatullin, you are too much in all this package change for no reason thing.

Let’s make a minor release and that’s it. Koptional is a Kotlin-first library, I see no harm in changing Java compatibility as a minor change. It is OK to change since it is no radical. Otherwise every library should follow Chrome or Firefox versions scheme and have versions in double or triple digits since API change all the time and hey, there are artifact versions exactly for that. Artem just has some kind of fashion taste in bumping package names (even for internal libraries with a single user, yep) :-)

Given that Koptional is Kotlin-first, I'd agree that this is a minor change, even though it breaks Java consumers. Changing the package name will require Kotlin consumers to change imports everywhere, and bumping the major version feels excessive for such a small change - compare it with RxJava 2 vs RxJava 1 or Dagger 2 vs Dagger 1. Even though it's breaking, it's trivial to fix.

It's trivial to fix for sure (basically find + replace for package name)

However because Koptional was initially designed to be very minimalistic it might be used by other libraries which means that in such a case we're breaking compatibility between user's code and other library.

That's why i suggest both major version bump and groupid update :)

GitHub search results: https://github.com/search?utf8=%E2%9C%93&q=com.gojuno.koptional&type=Code

Search shows no usage from Java code, so...

Which doesn't mean there are no private Java libs that depend on class name…

This change is still breaking even for @Egorand himself because they use Koptional from Java (that's the point of PR), which means that we should at least bump major version https://semver.org/

Do we need to add number to groupId and package name?
I think yes, it's very trivial to migrate to new one in same commit as dependency update and we allow users to migrate their own code and dependencies gradually in case of class-name problem

I might not agree with everything Jake thinks but http://jakewharton.com/java-interoperability-policy-for-major-version-updates/ is very good

Let’s assume we have more Kotlin users than Java users. Changing a package name will force all users to do a Find + Replace. Changing a Java name will do the same for Java users only. I’m in for the version change, but against the package change. I totally understand all your points and, believe me, I would be on your side regarding anything related to these kind of changes. But! This PR does not introduce any behavior changes. If .toNullable was changed, that’s for sure forces the package change. At the same time, the library is so tiny I cannot even believe we are arguing about this 😆

bensandee · 2018-03-19T17:13:05Z

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 · 2018-03-19T17:44:21Z

@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 · 2018-03-19T17:46:49Z

I sure hope it's a good drink :D

artem-zinnatullin · 2018-03-20T12:11:23Z

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 · 2018-03-26T20:38:32Z

Soooo?

artem-zinnatullin · 2018-04-04T02:10:12Z

Can I go ahead and:

Merge this PR
Make a PR with groupId and packageName update
Release v2
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 · 2018-04-04T06:54:45Z

@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 · 2018-04-04T19:21:17Z

Kool, implementing then 👍

artem-zinnatullin · 2018-04-04T19:23:59Z

@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

vanniktech

Should there also be a Java test to verify this change?

artem-zinnatullin · 2018-04-04T19:44:29Z

That'd be nice!

artem-zinnatullin · 2018-04-09T22:46:39Z

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

Egorand · 2018-04-10T01:39:20Z

Sure, will update the PR tomorrow

Egorand · 2018-04-10T20:14:50Z

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:

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.
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.
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 · 2018-04-11T01:03:51Z

Indeed, I don't see a way to solve this atm
Yep, looks good 👍
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 · 2018-04-11T01:30:08Z

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 · 2018-04-11T01:41:25Z

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 · 2018-04-11T02:04:00Z

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 · 2018-04-20T12:24:49Z

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 · 2018-04-20T12:29:50Z

So, actually, what do we agree on?

Egorand · 2018-04-20T13:10:50Z

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

dmitry-novikov · 2018-05-21T16:36:28Z

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

artem-zinnatullin · 2018-05-21T19:37:54Z

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 · 2018-05-21T20:26:16Z

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

Egorand · 2018-05-21T22:56:56Z

@artem-zinnatullin 0dafabb

artem-zinnatullin

Looks good, few comments

artem-zinnatullin · 2018-05-21T23:26:00Z

+    companion object {
+
+        /**
+         * Wrap an instance of T (or null) into an [Optional]:


supernit: Wrap[s]

artem-zinnatullin · 2018-05-21T23:26:36Z

+        /**
+         * Wrap an instance of T (or null) into an [Optional]:
+         *
+         * ```


Can we specify language here?

Like ```java

artem-zinnatullin · 2018-05-21T23:26:58Z


-fun <T : Any> T?.toOptional(): Optional<T> = if (this == null) None else Some(this)
+/**
+ * Wrap an instance of T (or null) into an [Optional]:


Same as above

artem-zinnatullin · 2018-05-21T23:29:08Z

+ * using the static [Optional.toOptional] method.
+ */
+@Suppress("NOTHING_TO_INLINE")
+inline fun <T : Any> T?.toOptional(): Optional<T> = if (this == null) None else Some(this)


Ah sorry for not being clear, that's not what I meant :)

Inlining this extension everywhere can inflate user's class files noticeably, we intentionally didn't make this one inline

What I was talking about is that we can copy-paste this body to the @JvmStatic one so we don't have to do two method calls just because we're calling it from Java

I got your point initially, just thought inlining would be a better option here, as opposed to duplication. Since it's a one-liner I don't see it noticeably affecting the bytecode size, but I can change it to just use the same code as the other version.

I see, well it's one-liner in Kotlin 😸 , bytecode is not that small

L0 LINENUMBER 21 L0 ALOAD 0 IFNONNULL L1 GETSTATIC com/gojuno/koptional/None.INSTANCE : Lcom/gojuno/koptional/None; CHECKCAST com/gojuno/koptional/Optional GOTO L2 L1 NEW com/gojuno/koptional/Some DUP ALOAD 0 INVOKESPECIAL com/gojuno/koptional/Some.<init> (Ljava/lang/Object;)V CHECKCAST com/gojuno/koptional/Optional L2 ARETURN L3 LOCALVARIABLE $receiver Ljava/lang/Object; L0 L3 0 MAXSTACK = 3 MAXLOCALS = 1

Wonder why it does CHECKCAST com/gojuno/koptional/Optional, seems useless, maybe we need to update compiler

Yeah well, hidden costs of Kotlin!

artem-zinnatullin · 2018-05-22T00:53:47Z

         */
        @JvmStatic
-        fun <T : Any> toOptional(t: T?): Optional<T> = t.toOptional()
+        fun <T : Any> toOptional(t: T?): Optional<T> = if (t == null) None else Some(t)


heh, parameter name t is part of public API in Kotlin, if you can come up with something more meaningful but short that'd be nice

I have obj on mind, but idk

Good catch! What about value?

Much better, let's use that!

artem-zinnatullin

Noice! Let's wait for review from the others now :)

artem-zinnatullin · 2018-05-31T02:28:33Z

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

Egorand · 2018-05-31T02:51:14Z

Sweet! Are you planning a release any time soon?

artem-zinnatullin · 2018-05-31T03:07:41Z

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

artem-zinnatullin · 2018-06-06T05:58:41Z

@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

dmitry-novikov approved these changes Mar 16, 2018

View reviewed changes

artem-zinnatullin suggested changes Mar 17, 2018

View reviewed changes

artem-zinnatullin added the enhancement label Mar 17, 2018

artem-zinnatullin self-assigned this Mar 17, 2018

arturdryomov approved these changes Mar 17, 2018

View reviewed changes

artem-zinnatullin mentioned this pull request Mar 20, 2018

Add "The Art of a Dependency Upgrade" post. arturdryomov/pages#2

Merged

artem-zinnatullin approved these changes Apr 4, 2018

View reviewed changes

vanniktech approved these changes Apr 4, 2018

View reviewed changes

Add static toOptional method to improve Java experience

3404519

Egorand force-pushed the egorand/jvm-name branch from f47cb99 to 3404519 Compare April 10, 2018 20:09

Add compat tests, add documentation, inline toOptional extension

0dafabb

artem-zinnatullin reviewed May 21, 2018

View reviewed changes

Uninline toOptional, improve KDoc

70126b6

artem-zinnatullin reviewed May 22, 2018

View reviewed changes

Rename Optional.toOptional parameter

aa8b028

artem-zinnatullin approved these changes May 22, 2018

View reviewed changes

Egorand changed the title ~~Add JvmName to Optional~~ Java interop version of toOptional() May 22, 2018

artem-zinnatullin merged commit 6ce979d into gojuno:master May 31, 2018

Egorand deleted the egorand/jvm-name branch May 31, 2018 02:50

Conversation

Egorand commented Mar 15, 2018

Uh oh!

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturdryomov Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bensandee commented Mar 19, 2018

Uh oh!

arturdryomov commented Mar 19, 2018

Uh oh!

vanniktech commented Mar 19, 2018

Uh oh!

artem-zinnatullin commented Mar 20, 2018

Uh oh!

artem-zinnatullin commented Mar 26, 2018

Uh oh!

artem-zinnatullin commented Apr 4, 2018

Uh oh!

arturdryomov commented Apr 4, 2018

Uh oh!

artem-zinnatullin commented Apr 4, 2018

Uh oh!

artem-zinnatullin commented Apr 4, 2018

Uh oh!

vanniktech left a comment

Choose a reason for hiding this comment

Uh oh!

artem-zinnatullin commented Apr 4, 2018

Uh oh!

artem-zinnatullin commented Apr 9, 2018

Uh oh!

Egorand commented Apr 10, 2018

Uh oh!

Egorand commented Apr 10, 2018

Uh oh!

artem-zinnatullin commented Apr 11, 2018

Uh oh!

Egorand commented Apr 11, 2018

Uh oh!

artem-zinnatullin commented Apr 11, 2018

Uh oh!

Egorand commented Apr 11, 2018

Uh oh!

arturdryomov commented Apr 20, 2018

Uh oh!

arturdryomov commented Apr 20, 2018

Uh oh!

Egorand commented Apr 20, 2018

Uh oh!

dmitry-novikov commented May 21, 2018

Uh oh!

artem-zinnatullin commented May 21, 2018

Uh oh!

Egorand commented May 21, 2018

Uh oh!

Egorand commented May 21, 2018

Uh oh!

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Uh oh!

arturdryomov Mar 17, 2018 •

edited

Loading