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

time.Duration not supported in v2 #767

Closed
twpayne opened this issue Apr 30, 2022 · 3 comments
Closed

time.Duration not supported in v2 #767

twpayne opened this issue Apr 30, 2022 · 3 comments
Labels
feature Issue asking for a new feature in go-toml.

Comments

@twpayne
Copy link

twpayne commented Apr 30, 2022

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

  • go-toml: version ed80712 (2.0.0)
  • go: 1.18.1
  • operating system: observed on macOS and Linux

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
Copy link
Owner

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 toml-lang/toml#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.

@pelletier pelletier added the feature Issue asking for a new feature in go-toml. label May 9, 2022
@twpayne
Copy link
Author

twpayne commented May 9, 2022

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 twpayne closed this as completed May 9, 2022
@twpayne
Copy link
Author

twpayne commented May 9, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Issue asking for a new feature in go-toml.
Projects
None yet
Development

No branches or pull requests

2 participants