feat: add terramate fmt #351

katcipis · 2022-05-16T18:59:50Z

Adds the terramate fmt command. It will by default format all files inside the current working dir + all subdirs recursively (the -recursive flag on terraform fmt). We can also run terramate fmt --check, in that case it only checks if there are any files to be formatted and if there is any exits with 1, 0 otherwise.

Both on terramate fmt and terramate fmt --check the list of the files changed will be sent to stdout.

Also changed all references to hclwrite.Format to use our own hcl.Format function, with some further changes on testing code since our hcl.Format only formats valid code, not anything =P

…-add-fmt

codecov · 2022-05-16T19:01:43Z

Codecov Report

Merging #351 (b9ab265) into main (1116995) will decrease coverage by 0.01%.
The diff coverage is 66.26%.

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
- Coverage   67.77%   67.76%   -0.02%     
==========================================
  Files          36       37       +1     
  Lines        6685     6844     +159     
==========================================
+ Hits         4531     4638     +107     
- Misses       1903     1952      +49     
- Partials      251      254       +3

Flag	Coverage Δ
tests	`67.76% <66.26%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/terramate/cli/cli.go	`5.54% <0.00%> (-0.28%)`	⬇️
generate/genhcl/genhcl.go	`93.44% <37.50%> (-4.04%)`	⬇️
test/hclwrite/hclwrite.go	`92.15% <50.00%> (-1.79%)`	⬇️
hcl/hcl.go	`86.29% <89.65%> (+0.50%)`	⬆️
hcl/fmt.go	`94.73% <94.73%> (ø)`
test/errors/errors.go	`44.82% <100.00%> (+7.57%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1116995...b9ab265. Read the comment docs.

katcipis · 2022-05-17T19:47:58Z

hcl/fmt_test.go

+	tcases := []testcase{
+		{
+			name: "attributes alignment",
+			input: `


when we start adding extra formatting features the idea is to add more test cases here

katcipis · 2022-05-18T14:59:49Z

hcl/eval/partial_fuzz_test.go

@@ -92,13 +90,9 @@ func FuzzPartialEval(f *testing.F) {

 		const testattr = "attr"

-		cfg := hcldoc(


Now hclwriter always produces valid HCL, so if you use it to build invalid HCL files it panics, seemed like the way to go since it is what we want on 99% of the tests, we use it to build some valid HCL file for testing. But here we generate possibly invalid HCLs and that would cause panics, so it seemed easier to just create the HCL with a string since it is a pretty straithforward attr = val.

katcipis · 2022-05-18T15:09:17Z

test/hclwrite/hclwrite.go

@@ -222,7 +222,11 @@ func NumberInt(name string, val int64) BlockBuilder {

 // Format formats the given HCL code.
 func Format(code string) string {
-	return strings.Trim(string(hclwrite.Format([]byte(code))), "\n ")
+	formatted, err := hcl.Format(code, "gen.hcl")
+	if err != nil {


One alternative to assuming that only valid HCL is allowed when using the hclwrite builders would be to just skip formatting if the code is not valid HCL (or maybe falling back to hclwrite.Format that formats anything). But the alternative seems clumsy, overall I think we want to build valid HCL using the builders, so if because a bug it is not valid HCL it is desireable for the tests to fail fast.

i4ki

lgtm but left minor ideas/questions.

i4ki · 2022-05-19T12:59:22Z

cmd/terramate/cli/cli.go

+
+	logger.Trace().Msg("listing formatted files")
+	for _, res := range results {
+		path := strings.TrimPrefix(res.Path(), c.wd()+"/")


filepath.Separator?

We have the prj.FriendlyFmtDir() but for formatting directories (we use them for stacks) and them they return directories relative to root. Maybe use this?

dir := prj.FriendlyFmtDir(c.root(), c.wd(), filepath.Dir(res.Path())) path := filepath.Join(dir, filepath.Base(res.Path()))

but not sure, ignore if doesn't make sense haha

Man I always forget about the filepath.Separator. I saw FriendlyFmtDir but as you mentioned it does something else and it seemed simpler to do just this (FriendlyFmtDir is more involved).

Using filepath.Separator. Maybe we could use prj.FriendlyFmtDir() but I'm not so sure 🤔

i4ki · 2022-05-19T12:59:43Z

cmd/terramate/cli/cli.go

+		c.log(path)
+	}
+
+	if c.parsedArgs.Fmt.Check {


i4ki · 2022-05-19T13:02:39Z

cmd/terramate/e2etests/fmt_test.go

+
+	wantedFiles := []string{
+		"globals.tm",
+		"another-stacks/globals.tm.hcl",


maybe add a test with --chdir into the generated tree to check if the file paths are expected?

Done, thanks for catching this (actually I was aware the tests were lacking and went into rush mode anyway, but it is good investment to improve that now, it didn't even take that long 😆 )

i4ki · 2022-05-19T13:04:57Z

hcl/fmt.go

+}
+
+const (
+	errFormatTree errors.Kind = "formatting tree"


we were placing this at the top in all other packages. Just curious why in the bottom in this one.

By default I never placed any private symbols/constants at the top =P. I'm just used to first listing all public things and then later all private things. But can also put on the top for consistency (not a strong opinion).

We are adding on the top all exported error sentinels/kinds etc. But this is just my default mode of organizing things, very personal, but I do make a distinction between a private vs public error sentinel, the private one can't even be used by caller for any checking, it is just to avoid duplication internally.

ah ok, all the others are public indeed.

…-add-fmt

katcipis added 13 commits May 13, 2022 16:19

refactor: extract predicate to check tm files

70949a6

test: add tests for ignored dotfiles on genhcl/file

331de56

refactor: use listTerramateFiles on parser

df90f3f

fix: code generation ignores dotfiles

1adb846

feat: add hcl.Format

b0a045e

feat: add hcl.FormatFile

015e69e

feat: add hcl.FormatTree

6a7e026

feat: finish hcl.FormatTree

2f51327

chore: add test list

a6def35

Merge branch 'main' of github.com:mineiros-io/terramate into katcipis…

69b1eca

…-add-fmt

feat: add new assert function

8b9e99a

chore: use new assert func on hcl test

45d1c60

feat: add proper validation on format

7008447

katcipis self-assigned this May 16, 2022

Merge branch 'main' into katcipis-add-fmt

0d53a2f

katcipis added 14 commits May 16, 2022 21:35

test: check that files are untouched

d21fec6

feat: add method to save formatting result

22c868b

refactor: rename FmtRes to FormatResult

1aea15f

test: check errors for all files

7160f5c

feat: hcl.FormatTree returns all errors on all files

dff097f

refactor: make formatting result immutable

47d1f1f

chore: improve docs

3911bd8

test: add CLI fmt first test

5cc555f

chore: add license info

2bbf873

feat: add fmt on CLI

622755b

test: add tests for empty/formatted project

22bf5cc

feat: check code that is already formatted

f22439e

refactor: remove unused sentinel

a7e4405

test: add more tree tests

54fa336

katcipis added 2 commits May 17, 2022 21:28

refactor: improve test name

8bda361

test: add error handling tests for dirs/files access

77e589f

katcipis marked this pull request as ready for review May 17, 2022 19:42

katcipis requested a review from i4ki May 17, 2022 19:42

chore: fix typo

0416c76

katcipis commented May 17, 2022

View reviewed changes

katcipis added 2 commits May 18, 2022 16:35

feat: use our hcl.Format on generate_hcl

fbf01da

test: use new Format function on test.hclwrite

41a8911

katcipis commented May 18, 2022

View reviewed changes

i4ki previously approved these changes May 19, 2022

View reviewed changes

katcipis added 2 commits May 19, 2022 15:11

Merge branch 'main' of github.com:mineiros-io/terramate into katcipis…

8e23219

…-add-fmt

fix: use filepath.Separator for portability

e2015f7

katcipis dismissed i4ki’s stale review via e2015f7 May 19, 2022 13:13

katcipis added 2 commits May 19, 2022 15:25

test: add check for subdir for fmt

0860d8b

test: add formatting on subdirs tests

b9ab265

katcipis requested a review from i4ki May 19, 2022 13:41

i4ki approved these changes May 19, 2022

View reviewed changes

katcipis merged commit 866c004 into main May 19, 2022

katcipis deleted the katcipis-add-fmt branch May 19, 2022 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add terramate fmt #351

feat: add terramate fmt #351

katcipis commented May 16, 2022 •

edited

codecov bot commented May 16, 2022 •

edited

katcipis May 17, 2022

katcipis May 18, 2022

katcipis May 18, 2022

i4ki left a comment

i4ki May 19, 2022

katcipis May 19, 2022

katcipis May 19, 2022

i4ki May 19, 2022

i4ki May 19, 2022

katcipis May 19, 2022

i4ki May 19, 2022

katcipis May 19, 2022

katcipis May 19, 2022

i4ki May 19, 2022

		@@ -92,13 +90,9 @@ func FuzzPartialEval(f *testing.F) {

		const testattr = "attr"

		cfg := hcldoc(

feat: add terramate fmt #351

feat: add terramate fmt #351

Conversation

katcipis commented May 16, 2022 • edited

codecov bot commented May 16, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i4ki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katcipis commented May 16, 2022 •

edited

codecov bot commented May 16, 2022 •

edited