refactor(dedup): second pass at src/ duplication — clear 10 of 13 remaining blocks #21

2026-05-27T17:12:17-05:00

bermudalamb commented

2026-05-27 17:12:17 -05:00

Summary

Follow-up to PR #20 — addresses the 13 duplicated_blocks SonarQube still reported after the first dedup pass. Five mechanical commits, plus two documented skips for cases that need architectural change beyond simple dedup.

Commit	Block(s) cleared	Approach
`507620c`	Snowflake.UpdateBreakdown ≡ SqlServer override (delete) + Insert TryParse-tail (helper)	Delete a redundant override + extract `ParsePreparation.TryMatchInsertSql`
`85cc79d`	PG/Snowflake QueryBreakdown ctor pair	New `StatementParser.PopulateClauseWithComments(rawText, ISqlClause)` instance method dedupes the `ExtractSqlComments` → set Clause/Comment pattern
`4b6b3ed`	PG StatementReader `<`/`>`/`\|`/`=` operator-handler self-dup	New `TryMatchTwoCharOperator(char, string)` helper. Reverses my earlier "readability tax" call after seeing the helper-based form is actually clearer
`a84fe97`	Markdown SqlServer/SqlStatement Generators 26-line block	Remove the local `TruncateText` 1-line forwarders; replace with `using static MarkdownTextHelpers;`

All 1180 tests stay green throughout.

Deliberate skips (documented for follow-up)

CTE WITH-clause generation (Snowflake.QueryBreakdown:475-498 ≡ SqlServer.QueryBreakdown:579-601, 24 lines): the duplicated block sits mid-method inside GetSqlBreakdown/GetSql, differs only by indent (4-space vs 5-space). Cleanly extracting requires pulling the WITH section into a protected virtual on the SqlServer base + parameterizing indent. Worth doing but its own commit.
Markdown CollectionGenerator pair (SqlServer:16-97 ≡ PostgreSql:17-98, 82 lines): each dialect's wrapper class calls collection.GetTotalSelectedColumns(), collection.GetParameterUsageReport(), etc. on a different concrete QueryBreakdownCollection. Each dialect has its own ParameterUsageReport class too. Real dedup needs a shared IHasQueryAnalysis<TQuery> (or similar) contract and unifying ParameterUsageReport. Architectural change — separate PR.

Test plan

dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release — 0 errors after each commit
dotnet test ... -c Release --no-build — 1180 passed, 0 failed
Gitea Actions sonarqube.yml scans this PR
Re-query /api/measures/component?metricKeys=duplicated_blocks,duplicated_lines,duplicated_lines_density — block count should drop from 13 to ~3 (the CTE block + the Markdown CollectionGenerator pair = 1+2 = 3)

## Summary Follow-up to PR #20 — addresses the 13 `duplicated_blocks` SonarQube still reported after the first dedup pass. Five mechanical commits, plus two documented skips for cases that need architectural change beyond simple dedup. | Commit | Block(s) cleared | Approach | |---|---|---| | `507620c` | Snowflake.UpdateBreakdown ≡ SqlServer override (delete) + Insert TryParse-tail (helper) | Delete a redundant override + extract `ParsePreparation.TryMatchInsertSql` | | `85cc79d` | PG/Snowflake QueryBreakdown ctor pair | New `StatementParser.PopulateClauseWithComments(rawText, ISqlClause)` instance method dedupes the `ExtractSqlComments` → set Clause/Comment pattern | | `4b6b3ed` | PG StatementReader `<`/`>`/`\|`/`=` operator-handler self-dup | New `TryMatchTwoCharOperator(char, string)` helper. Reverses my earlier "readability tax" call after seeing the helper-based form is actually clearer | | `a84fe97` | Markdown SqlServer/SqlStatement Generators 26-line block | Remove the local `TruncateText` 1-line forwarders; replace with `using static MarkdownTextHelpers;` | All 1180 tests stay green throughout. ## Deliberate skips (documented for follow-up) - **CTE WITH-clause generation** (Snowflake.QueryBreakdown:475-498 ≡ SqlServer.QueryBreakdown:579-601, 24 lines): the duplicated block sits mid-method inside `GetSqlBreakdown`/`GetSql`, differs only by indent (4-space vs 5-space). Cleanly extracting requires pulling the WITH section into a `protected virtual` on the SqlServer base + parameterizing indent. Worth doing but its own commit. - **Markdown CollectionGenerator pair** (SqlServer:16-97 ≡ PostgreSql:17-98, 82 lines): each dialect's wrapper class calls `collection.GetTotalSelectedColumns()`, `collection.GetParameterUsageReport()`, etc. on a different concrete `QueryBreakdownCollection`. Each dialect has its own `ParameterUsageReport` class too. Real dedup needs a shared `IHasQueryAnalysis<TQuery>` (or similar) contract and unifying `ParameterUsageReport`. Architectural change — separate PR. ## Test plan - [x] `dotnet build Strata.SqlTools.QueryBreakdown.sln -c Release` — 0 errors after each commit - [x] `dotnet test ... -c Release --no-build` — 1180 passed, 0 failed - [ ] Gitea Actions sonarqube.yml scans this PR - [ ] Re-query `/api/measures/component?metricKeys=duplicated_blocks,duplicated_lines,duplicated_lines_density` — block count should drop from 13 to ~3 (the CTE block + the Markdown CollectionGenerator pair = 1+2 = 3)

bermudalamb added 4 commits 2026-05-27 17:12:17 -05:00

refactor(dedup): redundant Snowflake override + shared Insert regex helper 507620cc04

Two follow-ups to the prior dedup pass:

- **Delete `Snowflake.UpdateBreakdown.GetSqlBreakdown`**: it was a
  byte-for-byte copy of the SqlServer base's `GetSqlBreakdown` (modulo
  one explanatory comment). Snowflake's UPDATE syntax — including the
  FROM clause — is identical at the formatter level, so the override
  was pure inheritance noise. Now inherits.

- **Extract `ParsePreparation.TryMatchInsertSql`**: the regex match +
  group extraction + failure message at the end of
  `SqlServer.InsertBreakdown.TryParse` and
  `Snowflake.InsertBreakdown.TryParse` was duplicated. Hoist the
  shared piece next to `TryRunPrelude` on `ParsePreparation`. Both
  callers continue to construct their own `InsertBreakdown` instance
  (the constructor signatures differ slightly between dialects).

All 1180 tests stay green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

refactor(dedup): share clause-with-comments ingestion (PG/Snowflake QueryBreakdown ctors) 85cc79d5a1

The `QueryBreakdown(string select, string from, ...)` constructors on
PostgreSql.QueryBreakdown and Snowflake.QueryBreakdown each ran the
same six-line pattern twice (once per clause): call
`parser.ExtractSqlComments`, set the clause to the trimmed result,
join the comment list into the Comment property.

New `StatementParser.PopulateClauseWithComments(rawText, target)`
instance method does both halves. Each ctor now reads:

    parser.PopulateClauseWithComments(selectClause, SelectClause);
    parser.PopulateClauseWithComments(fromClause, FromClause);

Same behavior; the helper is a pure refactor of existing semantics.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

refactor(dedup): TryMatchTwoCharOperator helper in PG StatementReader 4b6b3edc87

The PostgreSql-specific operator dispatch in
`StatementReader.TryHandleAdditionalCharacter` had four similar 4-7
line blocks (each handling a single-char operator with one or more
two-char variants — \<, \>, \|, \=). Sonar flagged it as a
self-duplication.

Extract a small `TryMatchTwoCharOperator(char, string)` helper that
encapsulates the "if next char matches, advance and emit two-char
operator" pattern. Each operator handler now reads as a small list:

    if (CurrentCharacter == '<')
    {
        MovePosition();
        if (TryMatchTwoCharOperator('=', "<=")) return true;
        if (TryMatchTwoCharOperator('>', "<>")) return true;
        if (TryMatchTwoCharOperator('<', "<<")) return true;
        _currentToken = new Token(TokenType.Operator, "<");
        return true;
    }

Reverses my earlier "extracting would obscure intent" call after
re-reading — the helper-based form actually surfaces the intent
("two-char operator dispatch") more clearly than the original.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

refactor(dedup): remove redundant local TruncateText forwarders

SonarQube Analysis / sonarqube (pull_request) Successful in 3m58s

Details

a84fe9768f

`QueryBreakdownGenerator`, `SqlStatementGenerator`, and
`ExpressionGenerator` each had a 1-line `private static string
TruncateText(...)` forwarder to `Internal.MarkdownTextHelpers.TruncateText`.
The forwarders existed only to keep existing call sites short
(`TruncateText(x, 50)` instead of the fully-qualified form).

Each file now imports `using static MarkdownTextHelpers;` once at the
top, so call sites continue to read identically and the local
forwarders are deleted. Removes the structural duplication Sonar was
flagging (two private static helpers — `EscapeMermaidText` + the
TruncateText forwarder — appearing in both `QueryBreakdownGenerator`
and `SqlStatementGenerator` with the same shape).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bermudalamb merged commit 620b85a61d into main

2026-05-27 17:14:01 -05:00

bermudalamb deleted branch chore/sonarqube-src-dedup-pass2

2026-05-27 17:14:01 -05:00

bermudalamb referenced this pull request

2026-05-27 17:28:50 -05:00

refactor(dedup): final pass — clear the last 4 src/ duplicate blocks #22

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Lambda-Associates/sql-utilities#21