Skip to content

Add ShowOption for configuring JSON decoding#427

Merged
kmoe merged 8 commits intomainfrom
bendbennett/issues-426
Dec 20, 2023
Merged

Add ShowOption for configuring JSON decoding#427
kmoe merged 8 commits intomainfrom
bendbennett/issues-426

Conversation

@bendbennett
Copy link
Contributor

Closes: #426

@bendbennett bendbennett added the enhancement New feature or request label Dec 8, 2023
}

// JSONConfig holds information which determines how JSON is decoded.
type JSONConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

Approved with naming convention suggestion

@kmoe kmoe merged commit 7975ca6 into main Dec 20, 2023
@kmoe kmoe deleted the bendbennett/issues-426 branch December 20, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider adding show option to configure JSON decoding

2 participants