Add new fmtcmd package (linting) #80

dcarley · 2016-01-06T21:55:53Z

This uses the printer package to normalise HCL files. It's inspired by
github.com/fatih/hclfmt - but it differs in that it's intended to be more
generic, re-usable, and follow the semantics of gofmt or go fmt.

I intend to utilise this in Terraform by implementing terraform fmt
sub-command which will normalise all files in the current working directory
(like go fmt ./..) or contents over STDIN (with terraform fmt - that
could be used by editor plugins). So that Terraform users can benefit from
linting without installing another package/binary.

I hope that by placing most of the logic in the HCL package it should be
easy to implement sub-commands in other projects that also use HCL.

Some notes about the implementation:

the significant difference from gofmt is that STDIN/STDOUT are passed in
and errors aren't logged/written directly to STDERR, which gives consumers
(e.g. Terraform) the ability to set appropriate exit codes
I chose to use inline fixtures instead of files because there were a
number of times where I needed to reference the contents and group them
together with diff output
it seemed simplest to construct the expected outputs by looping over the
relevant fixtures and building up a string/byte slice, hope it isn't too
confusing to read
the test failure reporting is kind of rough because the outputs are so
large, but I didn't want to add another diff function
I chose to have a separate test for sub-directories rather than making it
the default in the fixtures so that I didn't need to add additional logic
to the fixture rendering
the fixtures are sorted by filename before any of the tests runs so that
they match the order that they are read from disk by filepath.Walk()

This uses the `printer` package to normalise HCL files. It's inspired by github.com/fatih/hclfmt - but it differs in that it's intended to be more generic, re-usable, and follow the semantics of `gofmt` or `go fmt`. I intend to utilise this in Terraform by implementing `terraform fmt` sub-command which will normalise all files in the current working directory (like `go fmt ./..`) or contents over STDIN (with `terraform fmt -` that could be used by editor plugins). So that Terraform users can benefit from linting without installing another package/binary. I hope that by placing most of the logic in the HCL package it should be easy to implement sub-commands in other projects that also use HCL. Some notes about the implementation: - the significant difference from `gofmt` is that STDIN/STDOUT are passed in and errors aren't logged/written directly to STDERR, which gives consumers (e.g. Terraform) the ability to set appropriate exit codes - I chose to use inline fixtures instead of files because there were a number of times where I needed to reference the contents and group them together with diff output - it seemed simplest to construct the expected outputs by looping over the relevant fixtures and building up a string/byte slice, hope it isn't too confusing to read - the test failure reporting is kind of rough because the outputs are so large, but I didn't want to add another diff function - I chose to have a separate test for sub-directories rather than making it the default in the fixtures so that I didn't need to add additional logic to the fixture rendering - the fixtures are sorted by filename before any of the tests runs so that they match the order that they are read from disk by `filepath.Walk()`

dcarley · 2016-01-26T18:24:41Z

Any thoughts on this?

One thing that occurred to me, while implementing tests for the Terraform part, was whether STDOUT was the most appropriate thing to pass around? It looks like it might result in copying lines/strings to c.Ui.Output().

mitchellh · 2016-01-30T15:51:22Z

Looks great! I think the prototype for this function is correct and we should look at how to implement this in Terraform and worry about the UI there. The writer can always be a bytes buffer or IO pipe we handle separately there.

mitchellh · 2016-01-30T15:51:27Z

Sorry for the late response I totally missed this :)

Add new fmtcmd package (linting)

dcarley · 2016-01-30T20:33:13Z

Thanks! Terraform PR to follow soon..

echohack · 2016-06-27T19:23:15Z

This isn't terribly important, but can you comment on why vertical whitespacing is used? @dcarley

This is somewhat surprising to me because most languages I have seen either remove such whitespacing in their linters (python, json, rust) or have no opinion about it (ruby, go).

While I am a fan of having an automated standard for everyone to lint by, I'm not really a fan of hitting spacebar a ton. Maybe we can change this?

@dcarley