Skip to content

hclwrite: body.SetAttribute* return new attribute - #806

Open
bbasata wants to merge 5 commits into
mainfrom
bbasata/set
Open

hclwrite: body.SetAttribute* return new attribute#806
bbasata wants to merge 5 commits into
mainfrom
bbasata/set

Conversation

@bbasata

@bbasata bbasata commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Description

This change updates hclwrite.Body.SetAttribute* to return the new attribute rather than return nil.

This matches the intent in the documentation comments that indicate:

The return value is the attribute that was either modified in-place or created.

An available workaround:

body.SetAttributeRaw(name, tokens)
attr := body.GetAttribute(name)

Open question: I'm considering whether to check the return value of Body.SetAttribute* in hclwrite/ast_body_test.go.

bbasata added 2 commits June 15, 2026 17:58
This change updates Body.SetAttribute* to return the new attribute
rather than return nil.

This matches the intent in the documentation comments that indicate:

The return value is the attribute that was either modified in-place or
created.
@bbasata
bbasata marked this pull request as ready for review June 15, 2026 22:04
@bbasata
bbasata requested a review from a team as a code owner June 15, 2026 22:04
@bbasata

bbasata commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

TIL: #613

@bbasata

bbasata commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

Tests have been ... driven! These fail on main, as they should.

$ git checkout -b one main
Switched to a new branch 'one'
$ git cherry-pick bbasata/set
[one d135622] hclwrite: unit testing for Body.SetAttributeRaw
 Date: Wed Jul 29 13:11:25 2026 -0400
 1 file changed, 122 insertions(+)
$ go test ./hclwrite
--- FAIL: TestBodySetAttributeTraversal_ReturnsTheAttribute (0.00s)
    --- FAIL: TestBodySetAttributeTraversal_ReturnsTheAttribute/attribute_`one`_is_not_set_to_a_value (0.00s)
        ast_body_test.go:800: got: nil
            want: one=the.loneliest.number
--- FAIL: TestBodySetAttributeRaw_ReturnsTheAttribute (0.00s)
    --- FAIL: TestBodySetAttributeRaw_ReturnsTheAttribute/attribute_`one`_is_not_set_to_a_value (0.00s)
        ast_body_test.go:996: got: nil
            want: one="the loneliest number"
--- FAIL: TestBodySetAttributeValue_ReturnsTheAttribute (0.00s)
    --- FAIL: TestBodySetAttributeValue_ReturnsTheAttribute/attribute_`one`_is_not_set_to_a_value (0.00s)
        ast_body_test.go:1027: got: nil
            want: one="the loneliest number"
FAIL
FAIL    github.com/hashicorp/hcl/v2/hclwrite    0.570s
FAIL

Comment thread hclwrite/ast_body_test.go Outdated
//
// If there are no diagnostics, returns the value.
// If there are diagnostics, fails the test.
func must[E any](t *testing.T) func(value E, diags hcl.Diagnostics) E {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the nittiest of nitpicks I can pick (and I'm genuinely sorry for it), but what do you think about naming this something different? testFn instead of must, maybe?

We use "must" in terraform test fixtures to mean "this will panic if it fails" and "test" for test helpers, and while this obviously isn't terraform, consistency simplifies reviewing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

test + must -> trust? :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I typically want a must function to panic. In this case, I extracted an existing "do something, and t.Fatal on diagnostics" pattern from this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants