microsoft / microsoft/go-sqlcmd
vscode: migrate settings.json writer to full hujson AST manipulation
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 595
- Forks
- 91
- Avg merge
- 9h 35m
- Merged PRs (30d)
- 1
Description
Background
PR #688 added sqlcmd open vscode, which writes the user's settings.json to register a connection profile. The original implementation stripped JSONC comments and trailing commas, marshaled fresh JSON, and saved the user's original to a .sqlcmd-backup file. dlevy flagged that this clobbers the backup on every subsequent run (e.g. creating a second container loses the first backup) and silently destroys hand-authored comments.
PR #688 follow-up commit replaced the strip-and-marshal round trip with github.com/tailscale/hujson. We now parse the original AST and apply an RFC 6902 patch that touches only the two keys we own (mssql.connections, mssql.connectionGroups). Comments, trailing commas, and unrelated user keys round-trip untouched on replace operations, and the .sqlcmd-backup file is no longer needed.
Remaining limitation
hujson's Patch "add" operation (used when the document doesn't yet have an mssql.connections key) can blank a comment immediately adjacent to the insertion point. Unrelated keys are preserved; only a comment line at the insertion site is affected. This is a one-time event per settings.json (subsequent runs use the "replace" path, which fully preserves comments).
Additionally, createProfile / updateOrAddProfile in cmd/modern/root/open/vscode.go operate on map[string]interface{} for the connection array itself. This means per-connection comments inside mssql.connections[*] are not preserved when sqlcmd updates an existing profile.
Proposed work
Refactor cmd/modern/root/open/jsonc.go and the profile-construction helpers to operate directly on hujson.Value AST nodes instead of decoded maps. Specifically:
- Walk the parsed root
*hujson.Objectto findmssql.connectionsandmssql.connectionGroupsmembers, mutating theirValuein place rather than going through a JSON patch. - For the array of profiles, locate the existing profile member by
idand rewrite its*hujson.Objectfields one at a time, leaving member ordering and surrounding extras intact. - When inserting a new profile or a new top-level key, set
ObjectMember.Name.BeforeExtraexplicitly so the preceding comment isn't absorbed by hujson's leading-comment extraction.
Verification
The round-trip test in cmd/modern/root/open/jsonc_test.go (TestApplyJSONCSettingsUpdates_PreservesComments) should be extended to cover the add-comment-adjacent case, and a new test should verify per-connection comments inside the mssql.connections array survive an update to an existing profile.
References
- PR: #688
- Related review thread: https://github.com/microsoft/go-sqlcmd/pull/688#discussion_r3325235606
- hujson patch semantics: https://github.com/tailscale/hujson/blob/main/patch.go
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with cmd/modern/root/open/jsonc.go and cmd/modern/root/open/vscode.go to trace the current hujson parsing, profile construction, and update paths. Extend cmd/modern/root/open/jsonc_test.go, including TestApplyJSONCSettingsUpdates_PreservesComments, for add-adjacent and per-connection comments. Done means direct hujson AST updates preserve comments, ordering, and unrelated fields for new and existing profiles.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- cli, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 58/100