-
Notifications
You must be signed in to change notification settings - Fork 29
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
Adding Char SDK #541
Conversation
expected(65) = 'A' | ||
expected(66) = 'B' | ||
expected(0x6728) = '木' | ||
expected(-1) = '�' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
According to wikipedia that char is expected in the utf standard for
invalid bytes plus some other conditions, so I think what you have is
perfect.
…On Mon, Feb 26, 2024, 7:37 PM Michelle ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
examples/morphir-elm-projects/evaluator-tests/src/Morphir/Examples/App/CharTests.elm
<#541 (comment)>:
> + Test: Char/toCode
+ expected('A') = 65
+ expected('B') = 66
+ expected('木') = 0x6728
+
+-}
+charToCodeTest: Char -> Int
+charToCodeTest ch = toCode ch
+
+
+{-|
+ Test: Char/fromCode
+ expected(65) = 'A'
+ expected(66) = 'B'
+ expected(0x6728) = '木'
+ expected(-1) = '�'
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!
—
Reply to this email directly, view it on GitHub
<#541 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOLHQUF6H7ZEUVGWY77BSCLYVVIITAVCNFSM6AAAAABD3BSUGKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBSGQ2TKMJQGM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Adding support for char sdk. Currently symbols and unicode don't work as expected.