Skip to content

Fix DefaultSettingsXmlFactory.write() always emitting location comments#11924

Closed
utafrali wants to merge 1 commit into
apache:masterfrom
utafrali:fix/issue-11899-settings-location-comments
Closed

Fix DefaultSettingsXmlFactory.write() always emitting location comments#11924
utafrali wants to merge 1 commit into
apache:masterfrom
utafrali:fix/issue-11899-settings-location-comments

Conversation

@utafrali

Copy link
Copy Markdown
Contributor

Aligns DefaultSettingsXmlFactory.write() with the existing pattern in DefaultModelXmlFactory.write(). The writer now defaults to addLocationInformation = false and only enables it when an InputLocationFormatter is provided in the request.

Previously the SettingsStaxWriter constructor defaulted addLocationInformation to true, so every serialized settings document included location tracking comments regardless of whether a formatter was configured.

Closes #11899

Mirror the pattern from DefaultModelXmlFactory.write() to disable
location information by default and only enable it when an
InputLocationFormatter is provided via the request.

Closes apache#11899
@jhobertextra

Copy link
Copy Markdown

@utafrali Hi! Thanks a lot for opening this pull request. I really appreciate the effort. Before moving forward, could you add unit tests to cover the changes? That helps ensure the update behaves as expected.

@gnodet gnodet left a comment

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.

Claude Code on behalf of Guillaume Nodet

This PR is now superseded by #11984, which was merged on May 20, 2026 (commit 3e38c39). That PR contains the identical fix for DefaultSettingsXmlFactory.write(), and additionally:

  1. Fixes the same issue in DefaultToolchainsXmlFactory.write() (which this PR does not address)
  2. Includes comprehensive unit tests for both factories (DefaultSettingsXmlFactoryTest and DefaultToolchainsXmlFactoryTest)

The production code change in this PR is correct and follows the established pattern from DefaultModelXmlFactory.write(), but since #11984 has already landed on master with a broader scope, this PR can be closed.

Thank you @utafrali for identifying the bug and opening this PR -- you were first to submit a fix (April 10 vs April 24 for #11984). The analysis in issue #11899 was thorough and helpful.

@gnodet gnodet closed this May 21, 2026
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.

DefaultSettingsXmlFactory.write() Always Emits Location Comments

3 participants