engine: quote special identifiers in mdl writer#334
Conversation
MDL output was converting canonical identifiers to spaced names and emitting\nthem bare, which produced invalid MDL when names contained special\ncharacters like $, /, or |. This broke round-trip parsing for otherwise\nvalid models that rely on quoted identifiers.\n\nCentralize identifier formatting in the writer so every identifier path\n(expression emission, variable LHS, dimensions, and reconstructed stock\nflows) applies the same quoting/escaping rule. Keep multidimensional\narray element keys unquoted as tuples to preserve MDL roundtrip behavior,\nand add regressions for both special-character identifiers and tuple keys.
Code ReviewPR: engine: quote special identifiers in mdl writer I reviewed the full diff (160 additions, 19 deletions in SummaryThis PR adds a consistent identifier-quoting layer to the MDL writer. Previously, AnalysisNew functions are well-designed:
Call-site migration is consistent: All identifier emission paths in the equation/variable definition sections ( Intentionally unchanged call sites are correct:
Tests cover the key scenarios: special character quoting in variable references and entry names, expression composition with quoted identifiers, and multidimensional element key tuple preservation. FindingsNo bugs found. VerdictCorrect. The patch is well-structured, the quoting logic is sound, call-site migration is consistent and complete for its stated scope, and existing tests continue to pass alongside the new ones. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
==========================================
+ Coverage 79.68% 79.70% +0.02%
==========================================
Files 134 134
Lines 32820 32853 +33
==========================================
+ Hits 26152 26187 +35
+ Misses 6668 6666 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add unit tests for the escape logic in quoted identifiers (embedded quotes and backslashes) and for needs_mdl_quoting edge cases (empty strings, leading/trailing spaces, digit-start names). Document remaining underbar_to_space call sites with comments explaining why they do not use format_mdl_ident -- sketch element names use bare comma-delimited positional records where quoting is not applicable.
Code ReviewSummaryThis PR centralizes MDL identifier formatting by introducing FindingsNo bugs found. The logic in
Test coverage is good — edge cases (empty strings, leading/trailing whitespace, digit-leading names, embedded quotes/backslashes) are all exercised. The existing test suite validates that normal identifiers continue to work unchanged. Overall CorrectnessCorrect. The patch should not break existing code or tests, and it fixes a real gap where special-character identifiers produced invalid MDL output. |
Review cycle summaryTwo iterations of automated review (Codex + Claude) were run against this PR. The first iteration identified two improvements: (1) the escape path in The second iteration confirmed no remaining actionable issues from either reviewer. A suggestion for an integration-level round-trip test (write special-char identifiers, parse them back) was tracked as a follow-up issue. |
Summary
$,/, or|Test plan