Skip to content

Conversation

@atrico-go
Copy link

@atrico-go atrico-go commented Feb 26, 2025

Use of SelfClosingElement gets the indentation wrong
This is because EndElement handles this but it is not done for StartElement.
Very tiny change to fix this - call indent method after SelfClosingElement has been written.

@ydnar
Copy link
Contributor

ydnar commented Mar 3, 2025

Thanks! Can you add a test for this?

@ydnar ydnar self-assigned this Mar 3, 2025
@AtricoSoftware AtricoSoftware force-pushed the self-closing branch 2 times, most recently from 677df0e to 2b6bbb7 Compare March 17, 2025 09:47
@atrico-go
Copy link
Author

atrico-go commented Mar 17, 2025

Sorry, I missed your comment!
I've added the test and confirmed that it fails without the fix and passes with
(I do usually follow the TDD paradigm but in this case, my test was in my own (external) code so it didn't get added to the PR)

@case-fastly case-fastly requested a review from cee-dub March 27, 2025 02:27
Copy link

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

I've pulled the PR locally and run the tests both with and without the fix and I'm seeing what I would expect.

Here's the failing test when you remove the fix:

go test -v -run TestSelfClosingIndentation ./...
=== RUN   TestSelfClosingIndentation
    xml_test.go:2061: output mismatch:
        have: "<RootElement>\n   <Child1/>\n      <Child2/></RootElement>"
        want: "<RootElement>\n   <Child1/>\n   <Child2/>\n</RootElement>"
--- FAIL: TestSelfClosingIndentation (0.00s)
FAIL
FAIL    github.com/nbio/xml     0.559s
FAIL

Notice the indentation is doubled when there is no fix in place.

@cee-dub
Copy link

cee-dub commented Sep 9, 2025

Notice the indentation is doubled when there is no fix in place.

And there's a newline missing.

@Integralist Integralist merged commit c5d2563 into nbio:self-closing Sep 9, 2025
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.

5 participants