[2.13 RC1] Deprecation fixes for KotlinModule and docs for KotlinFeature

[TWiStErRob]

Note: this is not broken functionality, but highly improves DX of this deprecation. JetBrains: Develop with pleasure!™, right?

Original code: using all the deprecated methods

registerModule(
	KotlinModule.Builder()
		.apply {
			getNullIsSameAsDefault()
			getNullToEmptyMap()
			getNullToEmptyCollection()
			getStrictNullChecks()
			getSingletonSupport()
			reflectionCacheSize
		}
		.nullIsSameAsDefault(true)
		.nullToEmptyMap(false)
		.nullToEmptyCollection(false)
		.strictNullChecks(false)
		.singletonSupport(SingletonSupport.CANONICALIZE)
		.reflectionCacheSize(123)
		.build()
)

Using the IDE's deprecation quickfix.

Actual: errors, missing imports, values lost during deprecation replacement

registerModule(
	KotlinModule.Builder()
		.apply {
			isEnabled(NullIsSameAsDefault)
			isEnabled(NullToEmptyMap)
			isEnabled(NullToEmptyCollection)
			isEnabled(StrictNullChecks)
			isEnabled(SingletonSupport)
			reflectionCacheSize
		}
		.configure(NullIsSameAsDefault, enabled)
		.configure(NullToEmptyMap, enabled)
		.configure(NullToEmptyCollection, enabled)
		.configure(StrictNullChecks, enabled)
		.configure(SingletonSupport, enabled)
		.isEnabled(123)
		.build()
)

Expected (after this PR is merged): migrate to equivalent compiling code as much as possible

registerModule(
	KotlinModule.Builder()
		.apply {
			isEnabled(KotlinFeature.NullIsSameAsDefault)
			isEnabled(KotlinFeature.NullToEmptyMap)
			isEnabled(KotlinFeature.NullToEmptyCollection)
			isEnabled(KotlinFeature.StrictNullChecks)
			isEnabled(KotlinFeature.SingletonSupport)
			reflectionCacheSize
		}
		.configure(KotlinFeature.NullIsSameAsDefault, true)
		.configure(KotlinFeature.NullToEmptyMap, false)
		.configure(KotlinFeature.NullToEmptyCollection, false)
		.configure(KotlinFeature.StrictNullChecks, false)
		.configure(KotlinFeature.SingletonSupport, SingletonSupport.CANONICALIZE) // won't compile, but can't deduce
		.withReflectionCacheSize(123)
		.build()
)

• Finally, to have your code merged you will have to fill out the Contributor License Agreement and email a scan/photo of the result to info at fasterxml dot com.

[TWiStErRob]

CC @efenderbosch

Btw, did you want to fully publish this val? It was private, now it's public, internal?

enum class KotlinFeature(val enabledByDefault: Boolean)

[dinomite]

Superb additions!

[dinomite]

I don't think there is reason for enabledByDefault to not be private.

I'll wait a bit for response from @efenderbosch then merge; can always privatize it later.

[efenderbosch]

enabledByDefault can't be private because KotlinModule.Builder uses it to construct the default bitset.

It could be private if we construct the default bitset in KotlinFeature something like this:

enum class KotlinFeature(private val enabledByDefault: Boolean) {
    // snip
    companion object {
        internal val defaults = 0.toBitSet().apply {
            values().filter { it.enabledByDefault }.forEach { or(it.bitSet) }
        }
    }
}

and use it in KotlinModule.Builder something like this:

    class Builder {
        var reflectionCacheSize: Int = 512
            private set

        private val bitSet: BitSet = KotlinFeature.defaults
    // snip
}

But then KotlinFeature.defaults isn't immutable. That bitset could be modified and re-used w/ unexpected results. It can be internal though (recent edit), so that's not horrible.

[cowtowncoder]

CLA received & Filed successfully.

[TWiStErRob]

@efenderbosch indeed internal is the best one. I think internal APIs should be always internal, so that Kotlin users can't use them. Having something public, someone will use it; and if you change it, however internal it looked originally and protected with JavaDoc/@publicapi annotations/etc., they'll break.

I like your proposal of #477 (comment) you just need one tiny bit of adjustment to make it the way you want: internal val defaults get() =. This way you'll always get a clean mutable instance, and the enabledByDefault is encapsulated nicely while having the exact same performance characteristics as is on 2.13 now.

[dinomite]

@efenderbosch @TWiStErRob Great ideas, incorporated in 0793bbb

[TWiStErRob]

@dinomite looks like tests were reliant on that.