Skip to content

hclwrite: export LeadComments()/LineComments() - #807

Open
bbasata wants to merge 4 commits into
mainfrom
bbasata/comments
Open

hclwrite: export LeadComments()/LineComments()#807
bbasata wants to merge 4 commits into
mainfrom
bbasata/comments

Conversation

@bbasata

@bbasata bbasata commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Description

This change exports functions from the hclwrite package to read comments.

@bbasata
bbasata requested a review from a team as a code owner June 15, 2026 22:08
@mildwonkey

Copy link
Copy Markdown
Contributor

I got started writing tests for this and ran into a few things - let me know if any of these are just misunderstandings or testing wrong:

  1. Go formatted multi-line comments don't appear to be supported in lead comments, though it's working in line comments so it's very possible I messed up the test case. I can put

  2. Comments are returned with final newlines (maybe fine, this is my vaguest concern!)
    I'm not sure if the newlines at the end of the comment are appropriate (newlines within multi-line comments are expected but I assumed - not based on knowledge - that we'd trim the final newline, but that might make sense for comments in a way it doesn't for things like attributes 🤷🏻 ).

  3. Inline comments include a space before the comment indicator:
    test_attribute = foo # comment returns

# comment instead of # comment

  1. We lose the last line of (admittedly badly formatted) multi-line inline comments:
    Something like this:
attr = foo # i am a rather 
                 # long comment, and there's nothing else on this line
                 # so this line here will be returned as the lead line on the next attr, probably? or just get lost.
 other_attr = bar

Just returns the first line ( # i am a rather)

I'm not sure how we should approach "bad" multi-line inline comments (the ones not using actual multi-line syntax). I don't know if it's realistic to expect hclwrite to tell a multi-line inline comment apart from a single line inline comment + the next attribute's lead comment - is the indentation sufficient? does it have to be exactly indented, or just "more than the attribute is"? Do we even have that info?

I suspect it's sufficient to say that terraform's behavior is undefined if you use single-line comment indicators for multiline comments (as the comment may get lost or end up with the next attr), but we do need to be deliberate about the choice, and document (and test) the limits.

If it would be easier, I can push the tests to your branch so you can see the test cases; I'll leave that up to you!

@apparentlymart

apparentlymart commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

FWIW hclwrite currently tracks these "special" comment tokens in primarily so that it can take them into account when callers make changes to whatever the comments are attached to.

For example, if a caller deletes an attribute with comments attached to it then hclwrite wants to delete the comments at the same time, rather than leaving them behind attached to nothing, or attached to the wrong thing.

The existing rules about which comments get attached and which ones don't were therefore aimed at being good heuristics for which comments an author might expect to be removed along with an item that's being deleted. Other uses for these comments might want slightly different heuristics, but changing the existing rules in-place would cause functions like Body.RemoveAttribute to behave slightly differently than they did before.

For that "i am a rather" example, RemoveAttribute("attr") would already be doing something arguably "wrong" of course:

# long comment, and there's nothing else on this line
# so this line here will be returned as the lead line on the next attr, probably? or just get lost.
other_attr = bar

...so the current heuristic isn't good for this situation even when focused on the intended use-case. I'm not sure whether authors actually write comments line this in practice.

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