Add functions for formatting numbers into strings

[urandom]

The functions formatFloat, formatInt and formatUint correspond
to the equivalent functions in strconv

[cznic]

Nice! I had only briefly skimmed the PR - and it looks great. Small missing bits are support for QL types - int8, int16 and uint8(aka byte) and uint16. Can you please complete that (with test cases please)?

Also, please add yourself to the AUTHORS and/or CONTRIBUTORS file (please keep them sorted) and include that in the PR. Thank you!

One question: Have you considered making formatUint and formatInt just one function? A type switch is required/made inside it/them anyway and it would be perhaps a little bit easier to use...?

[cznic]

@urandom FYI: You now have write access to the QL repository.

[urandom]

One question: Have you considered making formatUint and formatInt just one function? A type switch is required/made inside it/them anyway and it would be perhaps a little bit easier to use...?

I thought of keeping the functionality somewhat on-par with the strconv functions. Though i guess it might be better if there was just one formatInt function. Guess I'll merge them.

[cznic]

Guess I'll merge them.

Please feel free to decide this minor point as you wish. I have no strong opinion on it, it was just an idea.

[urandom]

I've updated the PR to merge the 2 functions and handle all int types.

[cznic]

Thanks for the update. Everything looks fine, however tests do not pass for me.

$ make -B 2>&1 | tee log
sed -n '1,/^package/ s/^\/\/  //p' < doc.go \
        | ebnf2y -o ql.y -oe ql.ebnf -start StatementList -pkg ql -p _
goyacc -v /dev/null -o /dev/null ql.y
Parse table entries: 21946 of 70840, x 16 bits == 43892 bytes
goyacc -o parser.go -xe xerrors parser.y
Parse table entries: 21277 of 61305, x 16 bits == 42554 bytes
sed -i -e 's|//line.*||' -e 's/yyEofCode/yyEOFCode/' parser.go
golex -o scanner.go scanner.l
if [ -f coerce.go ] ; then rm coerce.go ; fi
go run helper.go | gofmt > coerce.go
go fmt
go test -i
go test
--- FAIL: TestMemStorage (0.34s)
    storage_test.go:320: FAIL: test # 791
        -- 791
        BEGIN TRANSACTION;
            CREATE TABLE t (c float32);
            INSERT INTO t VALUES (43.2);
        COMMIT;
        SELECT formatFloat(c) FROM t;

        ---- g
        s
        [43.20000076293945]
        ---- e
        s
        [43.2]
        ----
--- FAIL: TestFileStorage (189.96s)
    storage_test.go:320: FAIL: test # 791
        -- 791
        BEGIN TRANSACTION;
            CREATE TABLE t (c float32);
            INSERT INTO t VALUES (43.2);
        COMMIT;
        SELECT formatFloat(c) FROM t;

        ---- g
        s
        [43.20000076293945]
        ---- e
        s
        [43.2]
        ----
--- FAIL: TestOSFileStorage (190.26s)
    storage_test.go:320: FAIL: test # 791
        -- 791
        BEGIN TRANSACTION;
            CREATE TABLE t (c float32);
            INSERT INTO t VALUES (43.2);
        COMMIT;
        SELECT formatFloat(c) FROM t;

        ---- g
        s
        [43.20000076293945]
        ---- e
        s
        [43.2]
        ----
FAIL
exit status 1
FAIL    github.com/cznic/ql2    387.058s
make: *** [editor] Error 1
$  go version
go version go1.4.2 linux/amd64
$ 

The reason is

package main

import (
    "fmt"
    "strconv"
)

func main() {
    f32 := float32(43.2)

    fmt.Println(strconv.FormatFloat(float64(f32), 'g', -1, 64))
    fmt.Println(strconv.FormatFloat(float64(f32), 'g', -1, 32))
}

Output:

43.20000076293945
43.2

One line needs to be added. Original code:

func builtinFormatFloat(arg []interface{}, ctx map[interface{}]interface{}) (v interface{}, err error) {
    var val float64
    var fmt byte = 'g'

    prec := -1
    bitSize := 64

    switch x := arg[0].(type) {
    case nil:
        return nil, nil
    case float32:
        val = float64(x)
    case float64:
        val = x
    default:
        return nil, invArg(x, "formatFloat")
    }

Patch:

    case float32:
        val = float64(x)
        bitSize = 32

[urandom]

I added your patch.

I guess I didn't see the failed tests because go tests fails on the formatTime tests. I thought all test entries were invoked, regardless of whether any previous ones failed, since multiple formatTime tests were failing.

btw, the formatTime tests failed with these results:
---- g
sa
[2013-11-27 00:00:00 +0000]
[2013-11-27 15:07:00 +0200]
---- e
sa
[2013-11-27 00:00:00 +0000]
[2013-11-27 14:07:00 +0100]

It seems FormatTime assumes local time if the timezone wasn't specified in the layout.

[cznic]

Thank you for the update. The following changes make the tests pass for me.

diff --git a/builtin.go b/builtin.go
index 7797393..df1f064 100644
--- a/builtin.go
+++ b/builtin.go
@@ -388,27 +388,30 @@ func builtinFormatFloat(arg []interface{}, ctx map[interface{}]interface{}) (v i

    switch len(arg) {
    case 4:
-       switch y := arg[3].(type) {
+       arg3 := coerce1(arg[3], int64(0))
+       switch y := arg3.(type) {
        case nil:
            return nil, nil
-       case int:
-           bitSize = y
+       case int64:
+           bitSize = int(y)
        default:
            return nil, invArg(y, "formatFloat")
        }
        fallthrough
    case 3:
-       switch y := arg[3].(type) {
+       arg2 := coerce1(arg[2], int64(0))
+       switch y := arg2.(type) {
        case nil:
            return nil, nil
-       case int:
-           prec = y
+       case int64:
+           prec = int(y)
        default:
            return nil, invArg(y, "formatFloat")
        }
        fallthrough
    case 2:
-       switch y := arg[3].(type) {
+       arg1 := coerce1(arg[1], byte(0))
+       switch y := arg1.(type) {
        case nil:
            return nil, nil
        case byte:
@@ -431,8 +434,6 @@ func builtinFormatInt(arg []interface{}, ctx map[interface{}]interface{}) (v int
    switch x := arg[0].(type) {
    case nil:
        return nil, nil
-   case int:
-       intVal = int64(x)
    case int8:
        intVal = int64(x)
    case int16:
@@ -441,9 +442,6 @@ func builtinFormatInt(arg []interface{}, ctx map[interface{}]interface{}) (v int
        intVal = int64(x)
    case int64:
        intVal = x
-   case uint:
-       uintType = true
-       uintVal = uint64(x)
    case uint8:
        uintType = true
        uintVal = uint64(x)
@@ -462,11 +460,12 @@ func builtinFormatInt(arg []interface{}, ctx map[interface{}]interface{}) (v int

    switch len(arg) {
    case 2:
-       switch y := arg[1].(type) {
+       arg1 := coerce1(arg[1], int64(0))
+       switch y := arg1.(type) {
        case nil:
            return nil, nil
-       case int:
-           base = y
+       case int64:
+           base = int(y)
        default:
            return nil, invArg(y, "formatInt")
        }

Notes:

• Internally QL has no int or uint type in the sense what they mean in Go. QL's int is an alias for int64 and uint for uint64. QL does not have platform specific types as that is, well, not cross platform ;-)
• The coerce1 function coerces "ideal" constants to the type of the other argument, if possible. It works the same way as in Go.

It seems FormatTime assumes local time if the timezone wasn't specified in the layout.

I don't know why the tests pass for me and not for you. Admittedly, I have insufficient knowledge about timezones. Can you please explain if you know more about this? In any case, please fill an separate issue about what test case fails for you as it must be fixed as well.

I thought all test entries were invoked, regardless of whether any previous ones failed, since multiple formatTime tests were failing.

I think that should probably be the default. I'll add an issue for that, thanks for noticing.

[cznic]

@urandom Please feel free to let me know if I screwed something by already merging the PR (with a follow up patch). I felt that we may be in a deadlock ;-)

Thank you very much for your contribution!

[urandom]

Everything looks fine. I'll go create an issue about the failing parseTime tests.