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

Add MarshalText and UnmarshalText for Duration #270

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

metalmatze
Copy link
Member

This is mostly needed for https://github.com/alecthomas/kong.
Furthermore it should be useful for others too, as it implements stdlib interfaces.

@roidelapluie
Copy link
Member

model.Duration is an internal format, generally kong should use time.Duration. What would be the usecase?

@metalmatze
Copy link
Member Author

time.Duration doesn't support anything above hours. Therefore it would need a custom wrapper to support 1w, 1y and more.
We want to add flags that support these time ranges too.
As there's no direct dependency on anything and this change implements two stdlib interfaces I don't see this hurting.

@roidelapluie
Copy link
Member

roidelapluie commented Feb 8, 2021

I am not sure that I'd like to advise large projects like kong to rely on prometheus common given all the direct and indirect dependencies we have. We have complains like #255 already, and I do not want to amplify these. I agree that the code change is small, but it is also using exported interfaces only, such as this could as well be done downstream in kong with the same amount of code:

type promDuration model.Duration
* implement the marshallers

Would that be an option? (note that it would still have the issue pointed by #255)

@metalmatze
Copy link
Member Author

There is no direct dependency with both projects anywhere. Kong won't depend on Prometheus common either. It depends on the encoding.TextUnmarshaler (https://golang.org/pkg/encoding/#TextUnmarshaler) which is an abstract interface in the stdlib. The only place where we would have both dependencies interact is in an application choosing to use kong and use model.Duration as flag data type.

package main

import (
	"fmt"

	"github.com/alecthomas/kong"
	"github.com/prometheus/common/model"
)

var CLI struct {
	Retention model.Duration `name:"retention" default:"15d" help:"Retention for our app."`
}

func main() {
	kong.Parse(&CLI)
	fmt.Printf("retention: %d\n", CLI.Retention)
}

Only in this downstream application both will be used. Before that there's no dependency with neither as kong depends on encoding.TextUnmarshaler.

@roidelapluie
Copy link
Member

Okay, can you add a comment about which interface you are implementing?

Signed-off-by: Matthias Loibl <[email protected]>

This is mostly needed for https://github.com/alecthomas/kong.
Furthermore it should be useful for others too, as it implements stdlib
interfaces.
@metalmatze metalmatze force-pushed the duration-encoding-text branch from cf4d06a to ee8cd20 Compare February 9, 2021 11:24
if c.expectedString == "" {
expectedString = c.in
}
text, _ := d.MarshalText() // MarshalText returns hardcoded nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the future it would have been great to check for error, even if we only return nil.

@roidelapluie roidelapluie merged commit 3dae578 into prometheus:master Feb 9, 2021
@roidelapluie
Copy link
Member

Thanks!!

@metalmatze metalmatze deleted the duration-encoding-text branch February 9, 2021 14:55
This was referenced Mar 11, 2021
This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants