hashicorp / hcl2

Former temporary home for experimental new version of HCL
https://github.com/hashicorp/hcl
Mozilla Public License 2.0
373 stars 66 forks source link

hclwrite: Define an assign or comment chain as continuous until an empty new line occurs #132

Closed borisroman closed 4 years ago

borisroman commented 4 years ago

Hi!

It will be my first addition to the HCL2 language, so bear with me. I've created this PR with readability in mind. A thing that struck me was that a chain of assignments where one of the assignments is a multi-line value isn't aligned.

This PR aligns that case.

So the following snippet;

a = 1 # foo
bungle = (
    "bonce"
) # baz
zebra = "striped" # baz

will be formatted as follows;

a      = 1         # foo
bungle = (
  "bonce"
)                  # baz
zebra  = "striped" # baz

Looks better, right? That said, it obviously is up for debate.

Cheers

hashicorp-cla commented 4 years ago

CLA assistant check
All committers have signed the CLA.

borisroman commented 4 years ago

Hmm, running the tests locally doesn't give an error:

➜  hcl2 git:(feature/allign-assign-comment-in-block) ./.travis.sh
?       github.com/hashicorp/hcl2/cmd/hcldec    [no test files]
?       github.com/hashicorp/hcl2/cmd/hclfmt    [no test files]
?       github.com/hashicorp/hcl2/cmd/hclspecsuite  [no test files]
ok      github.com/hashicorp/hcl2/ext/dynblock  0.008s  coverage: 79.5% of statements
ok      github.com/hashicorp/hcl2/ext/include   0.006s  coverage: 60.0% of statements
ok      github.com/hashicorp/hcl2/ext/transform 0.006s  coverage: 27.8% of statements
ok      github.com/hashicorp/hcl2/ext/typeexpr  0.009s  coverage: 95.3% of statements
ok      github.com/hashicorp/hcl2/ext/userfunc  0.007s  coverage: 89.2% of statements
?       github.com/hashicorp/hcl2/extras/grammar    [no test files]
ok      github.com/hashicorp/hcl2/gohcl 0.009s  coverage: 86.9% of statements
ok      github.com/hashicorp/hcl2/hcl   0.010s  coverage: 56.8% of statements
ok      github.com/hashicorp/hcl2/hcl/hclsyntax 0.050s  coverage: 74.7% of statements
?       github.com/hashicorp/hcl2/hcl/hclsyntax/fuzz/config [no test files]
?       github.com/hashicorp/hcl2/hcl/hclsyntax/fuzz/expr   [no test files]
?       github.com/hashicorp/hcl2/hcl/hclsyntax/fuzz/template   [no test files]
?       github.com/hashicorp/hcl2/hcl/hclsyntax/fuzz/traversal  [no test files]
ok      github.com/hashicorp/hcl2/hcl/integrationtest   0.007s  coverage: 0.0% of statements
ok      github.com/hashicorp/hcl2/hcl/json  0.012s  coverage: 84.4% of statements
?       github.com/hashicorp/hcl2/hcl/json/fuzz/config  [no test files]
ok      github.com/hashicorp/hcl2/hcl/spectests 1.578s  coverage: 0.0% of statements
ok      github.com/hashicorp/hcl2/hcldec    0.012s  coverage: 65.6% of statements
?       github.com/hashicorp/hcl2/hcled [no test files]
ok      github.com/hashicorp/hcl2/hclpack   0.009s  coverage: 74.8% of statements
?       github.com/hashicorp/hcl2/hclparse  [no test files]
ok      github.com/hashicorp/hcl2/hcltest   0.007s  coverage: 33.0% of statements
ok      github.com/hashicorp/hcl2/hclwrite  0.021s  coverage: 86.7% of statements

Ahh, looks like we can't use kylelemons/godebug unless we move to >= go 1.10.x. https://github.com/kylelemons/godebug/commit/bde9f3c96d7aa052631925fa03b944627a335ff

borisroman commented 4 years ago

Waiting on https://github.com/hashicorp/hcl2/pull/133 to get a green build.

apparentlymart commented 4 years ago

Hi @borisroman! Thanks for working on this.

The idea of breaking the indentation/alignment context each time there's a multi-line bracket-type sequence was intentional with the rationale that each new level of indentation is distinct from the one above it, and so that we're usually aligning similarly-lengthed lines and thus we're less likely to get a strange outlier where everything gets pushed too far over.

There are certainly some funny edges that result from that, but they tend to be in combinations that we don't expect to arise in practice because they diverge from usual HCL style. Having a comment on the same line as a closing bracket on a line of its own is a good example of that: the conventional way to format that would be to put the comment before the multi-line construct on a line of its own so that it's clearer that it applies to the whole argument, and not just a part of it:

a = 1 # foo

# baz
bungle = (
  "bonce"
)                  

zebra  = "striped" # baz

With that said, I expect that merging this change would create some churn for existing configurations and so I'd rather only do that if the result were unambiguously better, rather than just different, in order to make that inevitable churn for existing configurations worthwhile. This one seems likely to rouse folks who disagree and want it switched back, and ultimately many of the decisions that this formatter makes are arbitrary and can't possibly please everyone. Therefore my inclination is not to change it.

Thanks again for working on this.

hashibot commented 4 years ago

Hello! :robot:

Thank you for working on this change proposal.

Further development of HCL 2 will now be done in the main HCL repository. For reasons of compatibility with existing Go callers still using GOPATH mode, the master branch still tracks HCL 1 at this time, but we'll be continuing development of HCL 2 in a branch. For more information, please see the Version Selection wiki page.

Unfortunately, we can't robustly perform an automatic migration of pull requests into the new repository, and so my human friends have asked me to close out all of the existing PRs to reflect that sadly they cannot be merged in their current state. :confounded:

If you have the time and motivation, we'd love to continue discussion of this and the other existing proposed changes in the hashicorp/hcl repository in a new PR targeting the hcl2 branch. We're sorry that we weren't able to merge this prior to the transfer, and now cannot automatically migrate your PR to the new repository.

The hashicorp/hcl2 repository will soon be archived to reflect the fact that no new development is planned here. The repository will remain in place for the time being so that existing callers can continue to use it while they plan a migration to the new import paths and to a stable release of HCL 2.