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

quick POC at non-Long numerics (encoding and decoding) #164

Merged
merged 7 commits into from
Dec 28, 2022

Conversation

aSemy
Copy link
Collaborator

@aSemy aSemy commented Nov 21, 2022

A quick stab at showing how non-Long numbers could be encoded and decoded.

#163

This is a very quick draft, I haven't spent a lot of time on it. I'm just sharing because I think the general approach is good.

This approach is based off the Kotlinx Serialization approach to handling JSON numbers

https://github.com/Kotlin/kotlinx.serialization/blob/dc950f5098162a3f8dd742d04b95f9a0405470e1/formats/json/commonMain/src/kotlinx/serialization/json/internal/StreamingJsonDecoder.kt#L281-L290

First the numbers are parsed as a Long or Double, as is already the case. Then ktoml will try to manually convert these numbers to the required type, but first verifying that the numbers are in range.

If you wanted to verify based on the raw string content, you could perhaps implement a function based on how KxS parses numbers: https://github.com/Kotlin/kotlinx.serialization/blob/dc950f5098162a3f8dd742d04b95f9a0405470e1/formats/json/commonMain/src/kotlinx/serialization/json/internal/lexer/AbstractJsonLexer.kt#L533-L584


val floatValue = doubleValue.toFloat()

if (floatValue.isInfinite()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Json apparently doesn't have any way to express infinity and NaN, so I guess that's why KxS has this exception. Toml does have inf and nan though

Copy link
Owner

Choose a reason for hiding this comment

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

Looks like I actually missed that, need to support it...

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

ktlint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@orchestr7
Copy link
Owner

@aSemy I will merge your part into #171 , I have made a simple part, could you please review?

@orchestr7 orchestr7 marked this pull request as ready for review December 28, 2022 09:43
@aSemy
Copy link
Collaborator Author

aSemy commented Dec 28, 2022

@aSemy I will merge your part into #171 , I have made a simple part, could you please review?

Of course! Thanks for asking

@aSemy
Copy link
Collaborator Author

aSemy commented Dec 28, 2022

POC completed - will be finalised in #171

@aSemy aSemy closed this Dec 28, 2022
@orchestr7
Copy link
Owner

orchestr7 commented Dec 28, 2022

no-no, let's not close it! I will merge it to add your great tests as a separate PR to make a functional PR less :)
❤️

I just want to make smaller PRs, so this PR will be good to split the logic

@orchestr7 orchestr7 reopened this Dec 28, 2022
@aSemy
Copy link
Collaborator Author

aSemy commented Dec 28, 2022

Ahh okay, I misunderstood 👍

}
}

testFails("${Char.MAX_VALUE.digitToInt() + 1}")
Copy link
Owner

Choose a reason for hiding this comment

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

kotlin.IllegalArgumentException: Char � is not a decimal digit

@@ -26,8 +26,8 @@ public abstract class TomlAbstractDecoder : AbstractDecoder() {
override fun decodeByte(): Byte = decodePrimitiveType()
override fun decodeShort(): Short = decodePrimitiveType()
override fun decodeInt(): Int = decodePrimitiveType()
override fun decodeFloat(): Float = decodePrimitiveType()
override fun decodeChar(): Char = decodePrimitiveType()
override fun decodeFloat(): Float = invalidType("Float", "Double")
Copy link
Owner

@orchestr7 orchestr7 Dec 28, 2022

Choose a reason for hiding this comment

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

Let's split this logic into several PRs, same with Char.

Actually Char is even more discussible, because there is no syntax in Toml for char literals, only for Strings... I am afraid that allowing Char we can make A HUGE pitfall for users here

Copy link
Collaborator Author

@aSemy aSemy Dec 29, 2022

Choose a reason for hiding this comment

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

Yeah, I copy and pasted Char in, but in retrospect I don't think it's clear how it should be handled. Is Char a number? Should 101 be treated as ascii and be decoded to A? (No, probably not.) What about decoding a to val c: Char?

EDIT: Ah, I see you made an issue #174

Copy link
Owner

@orchestr7 orchestr7 left a comment

Choose a reason for hiding this comment

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

lgtm with my changes, aSemy, thank you once again!

@orchestr7 orchestr7 merged commit f800f8f into orchestr7:main Dec 28, 2022
@aSemy aSemy deleted the non_long_numerics branch June 18, 2023 13:17
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.

3 participants