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

add functions for creating JsonPrimitives from unsigned numbers #2160

Merged
merged 3 commits into from Mar 1, 2023

Conversation

aSemy
Copy link
Contributor

@aSemy aSemy commented Jan 17, 2023

fix #2130

Adds JsonPrimitive() overloads for constructing JsonPrimitive from unsigned numbers

@@ -121,4 +121,54 @@ class JsonPrimitiveSerializerTest : JsonTestBase() {
assertEquals(string, jvmExpectedString)
}
}

@Test
Copy link
Contributor

@shanshin shanshin Mar 1, 2023

Choose a reason for hiding this comment

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

Please also add serialization tests, like JsonUnquotedLiteralTest

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've added parametrizedTest {}, like in JsonUnquotedLiteralTest

* The value will be encoded as a JSON number.
*/
@ExperimentalSerializationApi
public fun JsonPrimitive(value: UByte): JsonPrimitive = JsonPrimitive(value.toULong())
Copy link
Contributor

Choose a reason for hiding this comment

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

The other functions use nullable parameters. I think for unsigned types we should do the same.

Copy link
Member

Choose a reason for hiding this comment

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

I believe we shouldn't, and nullability for other functions is rather a mistake.
It will cause unnecessary boxing, while bringing no benefit whatsoever: for nullable types, Nothing? overload will be chosen

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 tried adding nullability, but I ran into issues: #2130 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@qwwdfsad, shouldn't we leave an explanatory comment so that in the future there will be no questions why a different approach is used?

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have

@qwwdfsad qwwdfsad removed the request for review from sandwwraith March 1, 2023 14:18
@aSemy aSemy force-pushed the feat/json_primitive_unsigned_numbers branch from 8188f0c to e668b06 Compare March 1, 2023 14:33
@qwwdfsad qwwdfsad requested a review from shanshin March 1, 2023 15:12
Copy link
Contributor

@shanshin shanshin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@shanshin shanshin merged commit 6246e0a into Kotlin:dev Mar 1, 2023
@aSemy aSemy deleted the feat/json_primitive_unsigned_numbers branch May 11, 2023 06:10
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.

None yet

3 participants