command/fmt: Restore some more-opinionated behaviors #26390
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously formatting was just a simple wrapper around hclwrite.Format. That remains true here, but the call is factored out into a separate method in preparation for making it also do some Terraform-specific cleanups in a future commit.
Previously we were just using hclwrite.Format, a token-only formatting pass. Now we'll do that via the full hclwrite parser, getting the formatting as a side-effect of the parsing and re-serialization. This should have no change in observable behavior as-is, but in a future commit we'll add some additional processing rules that modify the syntax tree before re-serializing it.
Codecov Report
|
mildwonkey
reviewed
Sep 28, 2020
| @@ -0,0 +1,27 @@ | |||
| variable "a" { | |||
There was a problem hiding this comment.
What do you think about adding an object type variable in here? I ask mostly for completion, as I'm someone who likes to read tests first when looking at code, and it'd be nice (even though the expected behavior seems pretty obvious) to have all that in here as well. You absolutely don't need to block merging on this request if you don't want to.
There was a problem hiding this comment.
I assume by this you mean some more examples of situations that currently aren't significantly changed by terraform fmt, which would then potentially help us as we add additional behaviors in future? If so, that sounds like a great idea and I'll put some in now before I merge this.
mildwonkey
reviewed
Sep 28, 2020
| return tokens | ||
| } | ||
|
|
||
| // Because this quoted syntax is from Terraform 0.11 and |
There was a problem hiding this comment.
Thank you for the excellent explanation here; I can already see this saving someone (me!) from making an unintentionally unfortunate change.
mildwonkey
approved these changes
Sep 28, 2020
In Terraform 0.11 and earlier, the "terraform fmt" command was very opinionated in the interests of consistency. While that remains its goal, for pragmatic reasons Terraform 0.12 significantly reduced the number of formatting behaviors in the fmt command. We've held off on introducing 0.12-and-later-flavored cleanups out of concern it would make it harder to maintain modules that are cross-compatible with both Terraform 0.11 and 0.12, but with this aimed to land in 0.14 -- two major releases later -- our new goal is to help those who find older Terraform language examples learn about the more modern idiom. More rules may follow later, now that the implementation is set up to allow modifications to tokens as well as modifications to whitespace, but for this initial pass the command will now apply the following formatting conventions: - 0.11-style quoted variable type constraints will be replaced with their 0.12 syntax equivalents. For example, "string" becomes just string. (This change quiets a deprecation warning.) - Collection type constraints that don't specify an element type will be rewritten to specify the "any" element type explicitly, so list becomes list(any). - Arguments whose expressions consist of a quoted string template with only a single interpolation sequence inside will be "unwrapped" to be the naked expression instead, which is functionally equivalent. (This change quiets a deprecation warning.) - Block labels are given in quotes. Two of the rules above are coming from a secondary motivation of continuing down the deprecation path for two existing warnings, so authors can have two active deprecation warnings quieted automatically by "terraform fmt", without the need to run any third-party tools. All of these rules match with current documented idiom as shown in the Terraform documentation, so anyone who follows the documented style should see no changes as a result of this. Those who have adopted other local style will see their configuration files rewritten to the standard Terraform style, but it should not make any changes that affect the functionality of the configuration. There are some further similar rewriting rules that could be added in future, such as removing 0.11-style quotes around various keyword or static reference arguments, but this initial pass focused only on some rules that have been proven out in the third-party tool terraform-clean-syntax, from which much of this commit is a direct port. For now this doesn't attempt to re-introduce any rules about vertical whitespace, even though the 0.11 "terraform fmt" would previously apply such changes. We'll be more cautious about those because the results of those rules in Terraform 0.11 were often sub-optimal and so we'd prefer to re-introduce those with some care to the implications for those who may be using vertical formatting differences for some semantic purpose, like grouping together related arguments.
apparentlymart
force-pushed
the
f-better-fmt
branch
from
c83f659
to
5ca8f07
Compare
|
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
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In Terraform 0.11 and earlier, the
terraform fmtsubcommand was very opinionated in the interests of consistency. While that remains its goal (similar togo fmtin the Go community), for pragmatic reasons Terraform 0.12 significantly reduced the number of formatting behaviors in thefmtsub command. We've held off on introducing 0.12-and-later-flavored cleanups out of concern it would make it harder to maintain modules that are cross-compatible with both Terraform 0.11 and 0.12, but with this aimed to land in 0.14 -- two major releases after 0.12 -- our new goal is to help those who find older Terraform language examples have them updated to the more modern idiom.More rules may follow later, now that the implementation is set up to allow modifications to tokens as well as modifications to whitespace, but for this initial pass the command will now apply the following small set of formatting conventions:
"string"becomes juststring. (This change quiets a deprecation warning.)anyelement type explicitly, so e.g.listbecomeslist(any)."${var.foo}"becomes justvar.foo. (This change quiets a deprecation warning.)Two of the rules above are coming from a secondary motivation of continuing down the deprecation path for two existing warnings, so authors can have two active deprecation warnings quieted automatically by
terraform fmt, without the need to run any third-party tools.All four of these rules match with current documented idiom as shown in the Terraform documentation, so anyone who has followed the documented style should see no changes as a result of this. Those who have adopted other local style will see their configuration files rewritten to the standard Terraform style, but it should not make any changes that affect the meaning of the configuration.
There are some further similar rewriting rules that could be added in future, such as removing 0.11-style quotes around various keyword or static reference arguments, but this initial pass focused on some rules that have been proven out in my third-party tool
terraform-clean-syntax, from which much of this commit is a direct port.For now this doesn't attempt to re-introduce any rules about vertical whitespace, even though the 0.11
terraform fmtwould previously apply such changes. We'll be more cautious about those, because the results of the rules in Terraform 0.11 were often sub-optimal and so we'd prefer to re-introduce those with some care to the implications for those who may be using vertical formatting differences for some semantic purpose, like grouping together related arguments.