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

Use boolean values instead of pointers #24

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

AlexanderYastrebov
Copy link
Contributor

Signed-off-by: Alexander Yastrebov alexander.yastrebov@zalando.de

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov
Copy link
Contributor Author

Similar to #18 this change gets rid of primitive value pointers making it simpler to use struct literals to define options.

@MicahParks
Copy link
Owner

MicahParks commented Nov 13, 2021

This does change the behavior of Options, as false is the default bool value and is a valid Options field value.

(0 can represent an uninitialized time.Duration because it's an invalid Options field value.)

It could be a breaking change if someone passed in multiple options through the variadic arguments with the first one being a pointer to true and the second one being unspecified.

However, I could change the function comment to note that.

I'll consider this while I'm AFK. And likely run the tests, linters, and accept the PR when I get the chance.

@MicahParks MicahParks mentioned this pull request Nov 13, 2021
@AlexanderYastrebov
Copy link
Contributor Author

It could be a breaking change

By default both flags are false in JWKs. This would be a breaking change if one used multiple options and former option set the flag while the latter reset (i.e. end result is false) like:

	t := true
	f := false
	jwks, err := keyfunc.Get(jwksURL, keyfunc.Options{RefreshUnknownKID: &t}, keyfunc.Options{RefreshUnknownKID: &f})

@MicahParks MicahParks merged commit 798c40e into MicahParks:master Nov 18, 2021
@AlexanderYastrebov AlexanderYastrebov deleted the bool-pointers branch November 18, 2021 13:28
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