Handle infinity and NaN for string.format()#172
Conversation
zombiezen
left a comment
zombiezen
left a comment
There was a problem hiding this comment.
Generally LGTM, thank you! Would you be willing to add some unit tests for this behavior in stringlib_test.go?
internal/lua/stringlib.go
Outdated
| spec = spec[:len(spec)-1] + string(c+('X'-'A')) | ||
|
|
||
| isUpper := (c >= 'A' && c <= 'Z') | ||
| if math.IsNaN(n) { |
There was a problem hiding this comment.
nit: mind changing this if/else-if into a switch?
There was a problem hiding this comment.
How would I turn this if else into a switch? From what I can see, switch only check one value at a time.
This is my first time writing Go code, so any advice is welcome.
There was a problem hiding this comment.
Example here: https://go.dev/tour/flowcontrol/11
Switch without a condition is the same as
switch true.
So each of the if conditions would become a case in a switch.
internal/lua/stringlib.go
Outdated
| } else { | ||
| s = "inf" | ||
| } | ||
| if math.IsInf(n, -1) { |
There was a problem hiding this comment.
Let's make this a little more direct: we already know that the number is one of the infinities:
| if math.IsInf(n, -1) { | |
| if n < 0 { |
HigherOrderLogic
force-pushed
the
fix/string-format
branch
2 times, most recently
from
28f0305 to
fdb25cc
Compare
HigherOrderLogic
force-pushed
the
fix/string-format
branch
from
fdb25cc to
1e23d2b
Compare
I have updated according to the review, and there's only this left. The issue is I dont find any test case for |
zombiezen
left a comment
zombiezen
left a comment
There was a problem hiding this comment.
Apologies, I had incorrectly remembered there being unit tests for string.format. I went ahead and added a table-driven test to check some other functionality as well as this change. LGTM, thank you!
2 participants
Fixes (partially) #78.