Add ShowOption for configuring JSON decoding #427
Conversation
bendbennett
force-pushed
the
bendbennett/issues-426
branch
from
f1474b3 to
309258f
Compare
tfexec/options.go
Outdated
| @@ -243,6 +243,19 @@ func GraphPlan(file string) *GraphPlanOption { | |||
| return &GraphPlanOption{file} | |||
| } | |||
|
|
|||
| // JSONConfig holds information which determines how JSON is decoded. | |||
| type JSONConfig struct { | |||
There was a problem hiding this comment.
Rather than a general-purpose JSONConfig struct, prefer a UseJSONNumber struct with a single boolean option.
The design intent behind the variadic options is for each one to be explicit, thus avoiding structs whose fields may have additions over time that the caller is unaware of.
Most options are implemented this way. ReattachConfig below is a bit of a special case because it represents structured information that happens to be encoded in a string env var then decoded to a (hopefully) identical struct on the other side.
I'm not sure UseJSONNumber should be an option at all - it should be the default. This would be a breaking change for consumers of terraform-exec and I'm not sure yet how best to handle that.
I think a reasonable compromise in the meantime would be to make UseJSONNumber a single-purpose option, and then change the default to true as an explicit breaking change later once we're sure (v1.0 at the latest). What we need to do to be sure - I don't know.
There was a problem hiding this comment.
Thanks for the feedback. Have renamed as suggested. Agree that having this option seems like a reasonable way forward for the time being, but would make sense for all numerical values to be treated as json.Number when it's time to have that breaking change.
Closes: #426