Skip to content

command/format: Correctly quote diff object keys#25443

Closed
alisdair wants to merge 1 commit intomasterfrom
alisdair/diff-render-quoted-keys
Closed

command/format: Correctly quote diff object keys#25443
alisdair wants to merge 1 commit intomasterfrom
alisdair/diff-render-quoted-keys

Conversation

@alisdair
Copy link
Copy Markdown
Contributor

When rendering a diff, we should quote object attribute names if the string representation is not a valid identifier. While this is not strictly necessary, it makes the diff output more closely resemble the configuration language, which is less confusing.

This commit applies to both top-level schema attributes and any object value attributes. We use a simplistic "%q" Go format string to quote the strings, which is not strictly identical to HCL's quoting requirements but is the pattern used elsewhere in HCL and Terraform.

Fixes #21749

When rendering a diff, we should quote object attribute names if the
string representation is not a valid identifier. While this is not
strictly necessary, it makes the diff output more closely resemble the
configuration language, which is less confusing.

This commit applies to both top-level schema attributes and any object
value attributes. We use a simplistic "%q" Go format string to quote the
strings, which is not strictly identical to HCL's quoting requirements,
but is the pattern used elsewhere in HCL and Terraform.
@alisdair alisdair self-assigned this Jun 30, 2020
@alisdair alisdair requested a review from a team June 30, 2020 22:14
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 30, 2020

Codecov Report

Merging #25443 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
command/format/diff.go 86.81% <100.00%> (+1.10%) ⬆️
terraform/node_resource_plan.go 91.80% <0.00%> (-1.64%) ⬇️
states/statefile/version4.go 57.87% <0.00%> (-0.29%) ⬇️
dag/marshal.go 54.44% <0.00%> (+1.11%) ⬆️

@alisdair alisdair added this to the v0.13.1 milestone Jul 7, 2020
Copy link
Copy Markdown
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks good to me! 🎉

I'm sure one day we'll refactor this a bit, cause we essentially "shipped the prototype" here for 0.12, but this seems like a good incremental fix to make the output less confusing, despite the fact that it was kinda awkward to implement in our current structure.

@alisdair alisdair modified the milestones: v0.13.1, v0.13.x Aug 25, 2020
@alisdair
Copy link
Copy Markdown
Contributor Author

This approach conflicts with some other upcoming work on the plan renderer, which is higher priority, so I'm not going to merge it at this time.

@alisdair alisdair closed this Aug 27, 2020
@ghost
Copy link
Copy Markdown

ghost commented Oct 13, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 13, 2020
@pselle pselle deleted the alisdair/diff-render-quoted-keys branch March 4, 2021 20:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

terraform plan produces invalid hcl when there are colons in property identifiers

2 participants