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

Options should use time values, but not pointer #18

Closed
nejtr0n opened this issue Sep 29, 2021 · 4 comments
Closed

Options should use time values, but not pointer #18

nejtr0n opened this issue Sep 29, 2021 · 4 comments

Comments

@nejtr0n
Copy link

nejtr0n commented Sep 29, 2021

Time package documentation advises to use values, but not pointers.
https://github.com/golang/go/blob/master/src/time/time.go#L85-L87

RefreshInterval *time.Duration

@MicahParks
Copy link
Owner

Yup. Typically time.Duration should be stored as a value, not a pointer.

However, currently in this project they're all stored as pointers. This is because the presence of the time.Duration fields are optionally used to configure the program.

If you've come across an instance where the code follows a pointer reference where it's possible it could be nil, please draw attention to it.

@nejtr0n
Copy link
Author

nejtr0n commented Sep 30, 2021

Imho we could test zero duration

type Test struct {
   duration time.Duration
}

data := &Test{}
fmt.Println(data.duration == 0)

@AlexanderYastrebov
Copy link
Contributor

The obvious (but not mentioned) problem with pointer values is that it prevents use of struct literal to init options like:

jwks, err := keyfunc.Get(url, keyfunc.Options{
    RefreshInterval: 1 * time.Minute,
})

@MicahParks
Copy link
Owner

MicahParks commented Nov 13, 2021

Please see the v0.10.0 release.

I plan on doing a v1.0.0 release sometime within the next few weeks. So anyone should feel free to raise issues about things that could be improved before the first major semver. If anyone is interested in the v1.0.0 discussion, feel free to participate here: #23

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

No branches or pull requests

3 participants