Feature/query editor sql formater#238
Conversation
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
PR #238 review — SQL Formatter in Query Editor
What it does
Adds a Format button + Format Options dialog to the Query Editor that runs the editor contents through Microsoft.SqlServer.TransactSql.ScriptDom (new SqlFormattingService), with persisted options in %LOCALAPPDATA%/PerformanceStudio/perfstudio_format_settings.json (new SqlFormatSettingsService). New reflection-driven FormatOptionsWindow exposes every SqlFormatSettings property as a DataGrid row.
Good
- Base branch is
dev— correct. - DataGrid style include is already registered in
App.axaml, so the newDataGridrenders fine. - Settings are versioned via
SqlVersion(80–170) with a sensible default. - No PlanAnalyzer rule changes, so no PerformanceMonitor Dashboard/Lite sync concerns.
- No
:pointeroverselectors;ToggleSwitch:checked /template/selectors are correctly template-part scoped. - Inline brushes in
FormatOptionsWindow.axamlare in the light-text / dark-bg range — no contrast regression against the#6B7280-style rejections.
Needs attention (inline comments)
- Theme brushes not reused:
FormatOptionsWindow.axamlhardcodes#1A1D23/#E4E6EBinstead of{DynamicResource BackgroundBrush}/{DynamicResource ForegroundBrush}. - Unused usings:
System.Collections.Generic+System.LinqinFormatOptionsWindow.axaml.cs;System.TextinSqlFormattingService.cs. - Silently swallowed errors:
Save_Clickcatch-all,int.ParsewithoutTryParse, and bothSqlFormatSettingsService.Load/Saveeat every exception. Corrupt settings or locked app-data = defaults forever, no user feedback. ItemsSource = nullrebuild in Revert_Click: symptom-fix; thePropertyChangedevents should already refresh bindings if the cell template is right.- Format button UX: full-document replace drops caret/selection/folding; tooltip advertises a
Ctrl+K, Ctrl+Dchord that isn't wired up. - Recoverable parse errors:
SqlFormattingService.Formatbails on any error, even when ScriptDom still returns a usable fragment. KeywordCasingenum parse: case-insensitiveEnum.TryParseon unchecked user strings can accept "0" etc. — addEnum.IsDefinedor just type the property as the enum.
Test coverage
SqlFormattingService and SqlFormatSettings round-tripping are both non-trivial and live in src/, but nothing was added under tests/PlanViewer.Core.Tests/. These services are pure and easy to unit test (format-then-reparse stability, settings JSON round-trip, version fallback). Worth adding a small suite — note that the services currently live in PlanViewer.App, so you may need a new PlanViewer.App.Tests project or move the services to PlanViewer.Core.
Not flagged
FormatOptionRow : INotifyPropertyChanged reads like MVVM creep at first, but QueryStoreGridControl.QueryStoreRow uses the same pattern for DataGrid row backing types, so this is consistent with existing conventions here.
Generated by Claude Code
| try | ||
| { | ||
| var prop = row.PropertyInfo; | ||
| object? value; | ||
|
|
||
| if (prop.PropertyType == typeof(bool)) | ||
| value = row.BoolValue; | ||
| else if (prop.PropertyType == typeof(int)) | ||
| value = int.Parse(row.CurrentValue); | ||
| else | ||
| value = row.CurrentValue; | ||
|
|
||
| prop.SetValue(settings, value); | ||
| } | ||
| catch | ||
| { | ||
| // Skip invalid values — keep default | ||
| } | ||
| } |
There was a problem hiding this comment.
Two issues in this block:
int.Parse(row.CurrentValue)throws on bad input (empty string, "4.5", typo). Useint.TryParseso you can either keep the default or surface feedback — silently discarding a typed value is bad UX.- The
catch { }is an empty swallow. If anything at all fails (reflection error, type mismatch), the user's saved settings just quietly don't contain that field and they have no idea why. At minimum write the exception toDebug.WriteLine/ logger, or set a status message on the dialog.
Generated by Claude Code
| OptionsGrid.ItemsSource = null; | ||
| OptionsGrid.ItemsSource = _rows; |
There was a problem hiding this comment.
The ItemsSource = null / reassign dance is a hack to force the grid to refresh. Since CurrentValue and BoolValue setters already raise PropertyChanged, bindings should update themselves. If they don't, the real cause is likely that the CellTemplate TextBlock (read-only state) isn't in an edit cell — which means display shows a stale value until you enter edit mode. Prefer fixing the binding mode/template over rebuilding ItemsSource.
Generated by Claude Code
|
|
||
| if (errors != null && errors.Count > 0) | ||
| { | ||
| SetStatus($"Format: {errors.Count} parse error(s) — {errors[0].Message}"); | ||
| return; | ||
| } | ||
|
|
||
| QueryEditor.Document.BeginUpdate(); | ||
| try | ||
| { | ||
| QueryEditor.Document.Replace(0, QueryEditor.Document.TextLength, formatted); | ||
| } | ||
| finally | ||
| { | ||
| QueryEditor.Document.EndUpdate(); | ||
| } | ||
| SetStatus("Formatted"); |
There was a problem hiding this comment.
Two UX notes:
- Replacing the whole document via
Replace(0, TextLength, formatted)resets caret position, selection, and collapses AvaloniaEdit's fold state. Consider saving the caret offset (or an anchor) before the replace and restoring it after, clamped to the new length. - The tooltip promises a
Ctrl+K, Ctrl+Dchord but I don't see that key binding wired up anywhere in this PR. Either add the binding on the control (seeQueryEditor.KeyBindings/KeyBindingwithKeyGesture) or drop the chord from the tooltip.
Generated by Claude Code
| @@ -0,0 +1,71 @@ | |||
| using System.Collections.Generic; | |||
| using System.IO; | |||
| using System.Text; | |||
There was a problem hiding this comment.
using System.Text; is unused — no StringBuilder or Encoding in this file. Drop it.
Generated by Claude Code
| public static SqlFormatSettings Load() | ||
| { | ||
| try | ||
| { | ||
| if (!File.Exists(SettingsPath)) | ||
| return new SqlFormatSettings(); | ||
|
|
||
| var json = File.ReadAllText(SettingsPath); | ||
| return JsonSerializer.Deserialize<SqlFormatSettings>(json, JsonOptions) ?? new SqlFormatSettings(); | ||
| } | ||
| catch | ||
| { | ||
| return new SqlFormatSettings(); | ||
| } | ||
| } | ||
|
|
||
| public static void Save(SqlFormatSettings settings) | ||
| { | ||
| try | ||
| { | ||
| Directory.CreateDirectory(SettingsDir); | ||
| var json = JsonSerializer.Serialize(settings, JsonOptions); | ||
| File.WriteAllText(SettingsPath, json); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Both Load() and Save() swallow every exception into catch { }. If the settings file is corrupt, or the local app data folder is not writable (locked by antivirus, network profile, etc.), the user silently gets defaults forever with no clue why. At minimum Debug.WriteLine(ex) the exception; better, bubble a load error up so the dialog can show "using defaults — settings file unreadable".
Also, JsonIgnoreCondition.WhenWritingNull is a no-op here since every property is a non-nullable value type or string — nothing would ever serialize as null. Safe to remove.
Generated by Claude Code
| var parser = GetParser(settings.SqlVersion); | ||
|
|
||
| using var reader = new StringReader(sql); | ||
| var fragment = parser.Parse(reader, out var errors); | ||
|
|
||
| if (errors != null && errors.Count > 0) | ||
| return (sql, errors); | ||
|
|
There was a problem hiding this comment.
parser.Parse(reader, out var errors) returns a non-null IList<ParseError> even on success (empty list), and T-SQL parsers can also produce a non-null errors list and a usable fragment for recoverable errors. The current logic returns the original text whenever errors is non-empty, which means anything with a minor warning won't format at all. Consider checking errors.Count == 0 || fragment != null and only bailing when the fragment is null / unrecoverable.
Generated by Claude Code
| { | ||
| AlignClauseBodies = AlignClauseBodies, | ||
| AlignColumnDefinitionFields = AlignColumnDefinitionFields, | ||
| AlignSetClauseItem = AlignSetClauseItem, | ||
| AsKeywordOnOwnLine = AsKeywordOnOwnLine, | ||
| IncludeSemicolons = IncludeSemicolons, | ||
| IndentSetClause = IndentSetClause, |
There was a problem hiding this comment.
Enum.TryParse<KeywordCasing>(KeywordCasing, true, out var kc) — works, but case-insensitive + arbitrary user input means "oops" (or worse, the string "0") will parse to the first enum value rather than fall through to the fallback. Fix: also check Enum.IsDefined after TryParse, or constrain the settings type to an enum instead of string.
Generated by Claude Code
Second-opinion addendum to the review aboveAgreeing with everything in the prior review. A few things it missed that are worth calling out: Worth adding
Minor
Second pass by Claude (in-session). |
|
…a popup message appears with the syntax error
|
OK fixed : |
|
Follow-up on the updates — nearly everything from both reviews is addressed cleanly. A few notes: Not a blocker, but worth a follow-upThe new error dialog in Not asking you to rip out every occurrence in this PR — just flagging that the code-behind theming debt exists and the new dialog adds to it. Could be a separate cleanup PR. Still outstanding from earlier reviews
None are blockers. Good work on the async format + error popup — nice call on preserving editor content when parse fails. Follow-up by Claude (in-session). |
…rce("BackgroundBrush")!
2. Close without dirty-state prompt : fixed
3. Tooltip Ctrl+K,Ctrl+D : removed
|
The 0.75 scale works for instance and i don't see any problem with it. Tentatives to fix with LayoutTransform failed, so i let it like it is |
|
Nice work on the fixes — most of the earlier review is addressed. A few small items still outstanding that look worth a follow-up commit before merge: Theme brush regressions (contradict the "Dynamic brushes" commit):
Format error handling:
Settings persistence feedback:
Re: the Not blocking on tests or selection/fold preservation — happy to ship those as follow-ups. Follow-up review by Claude (in-session). |
2 participants
What does this PR do?
Add a SQL Formater to the Query Editor with format options
Which component(s) does this affect?
How was this tested?
Describe the testing you've done. Include:
Checklist
dotnet build -c Debug)dotnet test)