Add configuration options to NewFactValue #260
Conversation
arbulu89
force-pushed
the
new-fact-value-string-conversion
branch
from
8c1c86d
to
7e68407
Compare
There was a problem hiding this comment.
LGTM for the logic,
Just a couple of comments on the code style.
We can also use the functional options pattern, having in the end something more ideomatic like
NewFactValue(thingstocast, WithSnakeCase(), WithStringCasting()) or NewFactValue(thingstocast) without any additional configuration, having the options as variadic arguments.
But this is up to you, we have two options for now, so as you wish.
The only things I request to change are in the comments.
Some examples of functional options pattern: https://charlesxu.io/go-opts/
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if conf.SnakeCaseKeys { | ||
| key = strcase.ToSnake(key) |
There was a problem hiding this comment.
Can we use a new scoped variable, not assigning to the key moving pointer in the for loop?
arbulu89
force-pushed
the
new-fact-value-string-conversion
branch
from
538d825
to
e044965
Compare
| stringConversion bool | ||
| } | ||
|
|
||
| type FactValuOption func(f *conf) |
There was a problem hiding this comment.
Did you mean FactValueOption here?
arbulu89
force-pushed
the
new-fact-value-string-conversion
branch
from
381563a
to
52b9988
Compare
Create
NewFactValueWithConfto enable configuration of theNewFactValuefunction.Initially only
SnakeCaseKeysandStringConversionoptions are added