Consolidate dialect duplication and make mypy strict pass cleanly#23
Merged
Merged
Conversation
Two behavior-preserving maintainability passes (728 unit tests unchanged, generated SQL byte-identical): Dialect de-duplication: - Move clear-majority shared methods to the Dialect base class as concrete defaults (write_string_literal, write_string_concat, write_duration, write_interval, write_timestamp_arithmetic, write_struct_close, write_type_name via a TYPE_MAP class attr). Dialects override only where they diverge. - Add validate_sql_field_name() in _utils and share it across DuckDB/ BigQuery/MySQL/SQLite; refactor convert_re2_to_posix to reuse _validate_regex_common; extract field_is_json() shared by _converter and _analysis. mypy strict clean (171 -> 0 errors): - Parametrize lark generics: Tree[Token] (via a TreeT alias), list[...], Interpreter[Token, None]. Model child nodes as Branch = Tree[Token] | Token to match lark's Tree.children, widening child-consuming helpers and adding a _children() helper. isinstance(..., Tree) runtime checks left untouched. - Fix six one-offs: replace two tuple-expression lambdas with named helpers, rename a float-branch variable, add a missing return annotation. - Drop the "pre-existing lark type errors are expected" note from CLAUDE.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two behavior-preserving maintainability passes. The 728 unit tests are unchanged and pass, generated SQL is byte-identical, and
mypy --strictnow reports 0 errors (was 171).1. Dialect de-duplication
The six dialect files repeated many byte-identical
write_*methods, so any change had to be hand-applied to every copy.Dialectbase class as concrete defaults:write_string_literal,write_string_concat(||),write_duration,write_interval,write_timestamp_arithmetic,write_struct_close, andwrite_type_name(via aTYPE_MAPclass attribute). Dialects override only where they genuinely diverge (e.g. BigQuery string escaping, MySQL/SparkCONCAT, SQLite interval modifiers).validate_sql_field_name()in_utilsshared by DuckDB/BigQuery/MySQL/SQLite (parameterized by reserved set + length + dialect label).convert_re2_to_posixto reuse_validate_regex_common(dropped ~60 duplicated validation lines).field_is_json()(inschema.py), shared by_converterand_analysis.Net duplicate-function clusters dropped 20 → 15 (remaining ones are 2–3-dialect minority overlaps, left explicit by design).
2. mypy strict clean (171 → 0)
All 171 errors were typing gaps, not runtime bugs.
Tree[Token](via aTreeTalias),list[...],Interpreter[Token, None].Branch = Tree[Token] | Tokento match lark'sTree.children, widening child-consuming helpers and adding a small_children()helper.isinstance(..., Tree)runtime checks were left untouched (parametrized generics can't be used inisinstance).def ... -> Nonehelpers, renamed a float-branch variable, added a missing return annotation.CLAUDE.md.Test plan
uv run pytest tests/ --ignore=tests/integration→ 728 passeduv run mypy src/pycel2sql/→ Success, no issues (was 171 errors)uv run ruff check src/ tests/→ cleantests/integration/) deferred to CI — no local container runtime available.🤖 Generated with Claude Code