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

Allow Kotlin's null literal in JSON DSL #1907

Merged
merged 8 commits into from
Jun 7, 2022

Conversation

lukellmann
Copy link
Contributor

Right now you can not really use Kotlin's null literal with the JSON DSL:

val obj = buildJsonObject {
    put("string", "string")
    put("boolean", true)
    put("number", 1)

    // Error: Overload resolution ambiguity. All these functions match.
    // - public fun JsonObjectBuilder.put(key: String, value: Boolean?): JsonElement? defined in kotlinx.serialization.json
    // - public fun JsonObjectBuilder.put(key: String, value: Number?): JsonElement? defined in kotlinx.serialization.json
    // - public fun JsonObjectBuilder.put(key: String, value: String?): JsonElement? defined in kotlinx.serialization.json
    put("null", null)
}

val arr = buildJsonArray {
    add("string")
    add(true)
    add(1)

    // Error: Overload resolution ambiguity. All these functions match.
    // - public fun JsonArrayBuilder.add(value: Boolean?): Boolean defined in kotlinx.serialization.json
    // - public fun JsonArrayBuilder.add(value: Number?): Boolean defined in kotlinx.serialization.json
    // - public fun JsonArrayBuilder.add(value: String?): Boolean defined in kotlinx.serialization.json
    add(null)
}

val p0 = JsonPrimitive("string")
val p1 = JsonPrimitive(true)
val p2 = JsonPrimitive(1)

// Error: Overload resolution ambiguity. All these functions match.
// - public fun JsonPrimitive(value: Boolean?): JsonPrimitive defined in kotlinx.serialization.json
// - public fun JsonPrimitive(value: Number?): JsonPrimitive defined in kotlinx.serialization.json
// - public fun JsonPrimitive(value: String?): JsonPrimitive defined in kotlinx.serialization.json
val p3 = JsonPrimitive(null)

All of these errors could be resolved by using a cast like null as String? or by using JsonNull directly.
However, this is somewhat irritating as other primitives that are supported by JSON (numbers, strings, true and false) can be used directly while null can't.

To resolve the overload resolution ambiguity and allow use of the null literal in the JSON DSL, this PR adds overloads for the JsonObjectBuilder.put(), JsonArrayBuilder.add() and JsonPrimitve() functions that take an argument of type Nothing?.

@lukellmann lukellmann changed the title Alow Kotlin's null literal in JSON DSL Allow Kotlin's null literal in JSON DSL Apr 20, 2022
@lukellmann
Copy link
Contributor Author

I haven't added @ExperimentalSerializationApi to the new functions yet, but I'm supposed to do this, right?

@sandwwraith sandwwraith self-requested a review April 21, 2022 15:20
@lukellmann
Copy link
Contributor Author

I haven't added @ExperimentalSerializationApi to the new functions yet, but I'm supposed to do this, right?

I added the annotations now.

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay, your PR slipped out of my attention. You were completely right with @ExperimentalSerializationApi. I think we can merge this after you address the comments

@@ -69,19 +69,24 @@ public fun JsonPrimitive(value: String?): JsonPrimitive {
return JsonLiteral(value, isString = true)
}

/** Returns [JsonNull]. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that this function is needed? It's much easier here to use JsonNull instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not strictly needed, but it feels more consistent to me.
But it's fine for me if you think this one is unnecessary and should be removed.

Copy link
Member

@sandwwraith sandwwraith Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, maybe it would be easier to discover. Then it's better to use doc style as in the other functions:

/**
* Creates [JsonNull].
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted the doc style 👍

@sandwwraith sandwwraith merged commit aedd915 into Kotlin:dev Jun 7, 2022
@sandwwraith
Copy link
Member

Thank you for your efforts!

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.

2 participants