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

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

Merged
merged 5 commits into from
Jul 19, 2021
Merged

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

merged 5 commits into from
Jul 19, 2021

Conversation

TWiStErRob
Copy link
Contributor

@TWiStErRob TWiStErRob commented Jul 18, 2021

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
image

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
image

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
Copy link
Contributor Author

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
Copy link
Member

Superb additions!

@dinomite
Copy link
Member

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
Copy link

efenderbosch commented Jul 19, 2021

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.

@dinomite dinomite merged commit 414327f into FasterXML:2.13 Jul 19, 2021
@cowtowncoder
Copy link
Member

CLA received & Filed successfully.

@TWiStErRob TWiStErRob deleted the deprecation_fixes branch July 19, 2021 19:33
@TWiStErRob
Copy link
Contributor Author

@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 added a commit that referenced this pull request Jul 20, 2021
Suggested by efenderbosch@github & TWiStErRob@github
#477 (comment)
@dinomite
Copy link
Member

@efenderbosch @TWiStErRob Great ideas, incorporated in 0793bbb

@TWiStErRob
Copy link
Contributor Author

@dinomite looks like tests were reliant on that.

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.

4 participants