To type overloads with diff nullability#1839
Conversation
Allex-Nik
force-pushed
the
to-type-overloads-diff-nullability
branch
from
f23c7ed to
325ac42
Compare
| assertNullabilityPreserved(converted, typeOf<IMG>()) | ||
| } | ||
|
|
||
| // @Test |
There was a problem hiding this comment.
toUrl/toURL throw NoClassDefFoundError. Seems like they have the same signature which causes a conflict in case insensitive environment
| * @return A new [DataFrame] with the values converted to [LocalDate]. | ||
| */ | ||
| @JvmName("toLocalDateFromTInt") | ||
| @JvmName("toLocalDateFromTIntNullable") |
There was a problem hiding this comment.
Should it be corrected like that?
There was a problem hiding this comment.
for correctness, yes, but that breaks binary compatibility :/ on the other hand, removing all these overloads also breaks it, so it's probably fine.
Since we're breaking binary compatibility anyway, maybe you could try removing as many @JvmNames as possible? Many won't be needed anymore now
| @Refine | ||
| @Converter(IMG::class, nullable = false) | ||
| @Interpretable("ToSpecificType") | ||
| public fun <T, R : URL?> Convert<T, URL>.toImg(width: Int? = null, height: Int? = null): DataFrame<T> = |
There was a problem hiding this comment.
Do we need the R?
There was a problem hiding this comment.
I think R should have been the second argument in Convert<>
There was a problem hiding this comment.
probably it should be removed indeed
| toJavaLocalDate(formatter = pattern?.let { DateTimeFormatter.ofPattern(pattern) }, locale = locale) | ||
| .convert(this.columns).toLocalDate() | ||
|
|
||
| @Deprecated( |
There was a problem hiding this comment.
I couldn't test these last 3 deprecated functions: when I use it in a test, the compile time schema of the converted dataframe contains String and String?. I didn't investigate it further because first I wanted to ask if we need to test them at all :)
There was a problem hiding this comment.
Hmm, they seem to miss compiler plugin annotations, not sure why
There was a problem hiding this comment.
it's ok to not test them
| @@ -1004,28 +978,6 @@ public fun <T> Convert<T, URL>.toIFrame( | |||
| public fun <T, R : URL?> Convert<T, URL?>.toImg(width: Int? = null, height: Int? = null): DataFrame<T> = | |||
There was a problem hiding this comment.
Should we make R the second argument in Convert?
public fun <T, R : URL?> Convert<T, R>.toImg...Would it be wrong to remove R instead?
public fun <T> Convert<T, URL?>.toImg...There was a problem hiding this comment.
R is unused, no? It can probably be removed, it might be a mistake :)
Allex-Nik
force-pushed
the
to-type-overloads-diff-nullability
branch
from
325ac42 to
9474385
Compare
3 participants
Fixes #1695
However, after the removal of the overloads, the compiler plugin produces the expected schema only since the version 2.4.0. So it's probably better to postpone merging this PR till the 2.4.0 release.