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

Adding Char SDK #541

Merged
merged 12 commits into from
Feb 27, 2024
Merged

Adding Char SDK #541

merged 12 commits into from
Feb 27, 2024

Conversation

michelchan
Copy link
Contributor

Adding support for char sdk. Currently symbols and unicode don't work as expected.

expected(65) = 'A'
expected(66) = 'B'
expected(0x6728) = '木'
expected(-1) = '�'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a funny one, I don't think code points are defined on negative values, are they? Depending on the implementation this might be some random codepoint based on the current int === int32 assumptions in our SDK code.

Elm repl says


> Char.toCode <Char.fromCode -|Char.fromCode - Char.fromCode -1
65533 : Int
> Char.toCode <| Char.fromCode -1
65533 : Int

I don't know what 2^16 - 3 says about the implementation of elm...

Anyway, the point is, this test looks on the surface to be thorough, but maybe is actually encoding an expectation of undefined behavior.

Longer explanation: -1 is 0xffff...ff in binary, for however many bits your size is. UTF-8 is variable length. So instead of saying "give me 8 bytes," it says, "depending on the start of the bytes I see, I will know how many bytes you're giving me." I don't think the all-ones is a valid sequence in UTF-8, so this might be an invalid code point. Here we're more testing what weird behavior whatever underlying java or elm is doing in undefined edge cases rather than fixing desired behaviors. Ideally, the tests should lock down just desired behaviors to allow refactoring more easily.

AAaaaall that being said, I don't mind if you leave this in.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your comment in the implementation now, seems like you know more about this than I. Sorry for telling you stuff you know already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, I totally agree it's funky, but this was part of the examples in the morphir-elm docs so I just added them as is. I actually don't know what the right way of implementing toCode and fromCode is in Scala, so if you have any ideas that would be great!

Copy link
Contributor

@justin-corn justin-corn left a comment

Choose a reason for hiding this comment

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

lgtm

@justin-corn
Copy link
Contributor

justin-corn commented Feb 27, 2024 via email

@michelchan michelchan merged commit 076fa9a into finos:main Feb 27, 2024
15 checks passed
@michelchan michelchan deleted the char branch February 27, 2024 15:46
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