pelletier / go-toml

Go library for the TOML file format
https://github.com/pelletier/go-toml
Other
1.7k stars 209 forks source link

`time.Duration` not supported in v2 #767

Closed twpayne closed 2 years ago

twpayne commented 2 years ago

Describe the bug

Firstly, thank you for this excellent library!

go-toml v1 has builtin support for time.Durations (see #248 for most of the implementation). From my reading of the code of v2, there is currently no support for time.Duration and, without support for UnmarshalTOML, no way for users of the library to add support for it.

To Reproduce

Try to unmarshal a string defined by time.ParseDuration (like "1s") into a time.Duration field. I get the error toml: cannot store TOML string into a Go int64.

Expected behavior

I expect this to work (e.g. "1s" unmarshals to time.Second)

Versions

Additional context

I'll happily submit a PR to implement this. It might also be super quick for you to do it.

Refs #237 #248 #488.

pelletier commented 2 years ago

Thank you for bringing this up! I initially removed durations because they are not part of the toml spec. A custom type implementing encoding.TextUnmarshaler can have a similar effect:

package main

import (
    "fmt"
    "time"

    "github.com/pelletier/go-toml/v2"
)

type Duration time.Duration

func (d *Duration) UnmarshalText(b []byte) error {
    x, err := time.ParseDuration(string(b))
    if err != nil {
        return err
    }
    *d = Duration(x)
    return nil
}

type MyDoc struct {
    Thing Duration
}

func main() {
    var doc MyDoc
    data := `thing = "2s"`
    toml.Unmarshal([]byte(data), &doc)
    fmt.Printf("%v", time.Duration(doc.Thing))
}

https://go.dev/play/p/BRqC7LUzjta

Totally understand this is a bit awkward to manipulate, but feels like a reasonable trade-off for additional complexity in the library for a feature with an unclear amount of use. However, I just saw https://github.com/toml-lang/toml/issues/514. It sounds like there is some interest in implementing durations directly in the language. Especially, the last comment states:

20% of all TOML users already uses durations

Ideally I'd love to see if it's possible to nudge the spec in that direction (doesn't necessarily need to land, but more recent activity from the owners of toml-lang/toml would be great). If not, I could be convinced to implement it if there is a way to not complexity the lexer/parser too much. I'm a bit worried about the fact that it adds an other case to "i'm going over digits but i don't know if it's a number, a float, a date, a time, etc". Also would need to make it clear that such feature would not be covered under the no-backward compatibility break rule, since its syntax might change if the spec does include it at some point.

twpayne commented 2 years ago

Thank you very much for this - this allows me to upgrade to v2 immediately. I'd somehow missed the possibility using encoding.TextUnmarshaler - it's clearly in the docs.

twpayne commented 2 years ago

For me, this issue is closed, but of course re-open it if you want to keep this as a potential feature.