feat: align Lua encode with cjson#108
Conversation
📝 WalkthroughWalkthroughThe PR refactors ChangesEncoder Refactor for lua-cjson Alignment
Sequence DiagramNo sequence diagram is needed for this PR; the changes are primarily dispatch logic refactoring and table-classification decision trees, which are better represented by the flowchart in the review stack artifact. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a89705a to
37c65f2
Compare
a97577f to
67e2766
Compare
67e2766 to
0fa8886
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lua/qjson/table.lua (1)
706-723: 💤 Low valueUnreachable TABLE_TYPE_HINT branches (optional cleanup).
The
hint == "object"andhint == "array"branches at lines 712-717 appear unreachable:
TABLE_TYPE_HINTis only written at line 433 with value"array", immediately after settingempty_array_mtat line 432.- Since
empty_array_mtis checked first (line 708), tables withTABLE_TYPE_HINT["array"]will never reach line 715.TABLE_TYPE_HINTis never set to"object"anywhere in this file, and as a local, it cannot be set externally.If this is intentional for future extensibility, consider adding a brief comment. Otherwise, these branches could be removed to simplify the code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/qjson/table.lua` around lines 706 - 723, The branches in encode_plain_table that test TABLE_TYPE_HINT for "object" and "array" are unreachable given TABLE_TYPE_HINT is only ever set to "array" when empty_array_mt is assigned and empty_array_mt is checked first; either remove the hint branches to simplify encode_plain_table (leaving the empty_array_mt check and the classify_plain_table fallback) or, if you intend TABLE_TYPE_HINT for future use, add a concise comment above TABLE_TYPE_HINT and/or inside encode_plain_table explaining it’s reserved for extensibility so reviewers know the branches are intentional; reference the symbols TABLE_TYPE_HINT, encode_plain_table, empty_array_mt, and classify_plain_table when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lua/qjson/table.lua`:
- Around line 706-723: The branches in encode_plain_table that test
TABLE_TYPE_HINT for "object" and "array" are unreachable given TABLE_TYPE_HINT
is only ever set to "array" when empty_array_mt is assigned and empty_array_mt
is checked first; either remove the hint branches to simplify encode_plain_table
(leaving the empty_array_mt check and the classify_plain_table fallback) or, if
you intend TABLE_TYPE_HINT for future use, add a concise comment above
TABLE_TYPE_HINT and/or inside encode_plain_table explaining it’s reserved for
extensibility so reviewers know the branches are intentional; reference the
symbols TABLE_TYPE_HINT, encode_plain_table, empty_array_mt, and
classify_plain_table when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dec16cb1-076a-43eb-b386-c66d24459ca9
📒 Files selected for processing (5)
lua/qjson/table.luatests/lua/encode_cjson_compat_spec.luatests/lua/encode_depth_spec.luatests/lua/encode_errors_spec.luatests/lua/lazy_mutation_property_spec.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/lua/encode_depth_spec.lua
Summary
qjson.encodewith lua-cjson for nil, sparse arrays, and numeric table keysqjson.encode: circular reference, while preservingqjson.encode: max depth exceededfor truly deep acyclic inputsCloses #106
Tests
PATH="/tmp/lua-qjson-luarocks-5.1/bin:$HOME/.luarocks/bin:$PATH" make testcargo test --release --no-default-featurescargo test --features test-panic --releasemake lintPATH="/tmp/lua-qjson-luarocks-5.1/bin:$HOME/.luarocks/bin:$PATH" make lua-lintSummary by CodeRabbit
Release Notes
Bug Fixes
Improvements