fix: Extend multi-arity support to get and get-in

[andreasronge]

Summary

Implement multi-arity functions for get and get-in to match the pattern introduced in PR #160. This allows LLM-generated programs to call these functions with either 2 or 3 arguments.

Changes:

• Updated lib/ptc_runner/lisp/env.ex lines 72-73 to use {:multi_arity, {fn/2, fn/3}} pattern
• Added comprehensive tests for both arities and error cases

Test Results

✅ All 970 tests passing
✅ Format check passed
✅ Compilation with warnings-as-errors passed
✅ Credo linting passed

Fixes #161

[github-actions]

PR Review: Extend multi-arity support to get and get-in

Summary

Straightforward implementation that extends multi-arity support to get and get-in functions, following the pattern established in PR #160 for sort-by. The implementation is correct and tests are comprehensive.

What's Good

• Follows established pattern: Uses the same {:multi_arity, {fn/2, fn/3}} tuple structure as sort-by
• Comprehensive test coverage: Tests both arities for get and get-in, plus arity error cases
• Tests follow existing structure: New tests are placed in the correct describe "multi-arity builtins" block and follow the same pattern as sort-by tests
• Strong assertions: Tests assert specific values ("Alice", "default", nil) rather than just checking shapes
• Documentation already exists: docs/ptc-lisp-specification.md already documents the 3-arity signatures

Issues (Must Fix)

None.

Suggestions (Optional)

1. Test for get with nil map - Nice-to-have

   • Suggestion: The Runtime.get/2 and Runtime.get/3 handle nil maps (lines 125, 128), but this isn't tested
   • Benefit: Ensures edge case behavior is documented via tests
   • Example: (get nil :key) → nil, (get nil :key "default") → "default"
   • Complexity: Trivial (same pattern as existing tests)
   • Recommendation: Could be added in a follow-up PR if desired

2. Known get-in/3 nil semantics issue - Out of scope for this PR

   • As noted in issue [From PR #160] Extend multi-arity support to get and get-in #161's review, get_in/3 returns the default when the found value is nil, not just when the path is missing. This differs from Clojure semantics.
   • This is a pre-existing issue in Runtime.get_in/3 (lines 132-137) and should be addressed in a separate issue if desired.

Security

No concerns. The change only exposes existing, already-implemented functions.

Documentation

No updates needed. The docs/ptc-lisp-specification.md already documents both 2-arity and 3-arity signatures for get and get-in (lines 745-748).

Verdict

Approve - Clean implementation following established patterns with comprehensive tests. The code change is minimal (2 lines in env.ex) and the tests cover the expected behavior thoroughly.

[andreasronge]

@claude please fix these issues from PR review:

Issue 1: Add test for get with nil map

Location: test/ptc_runner/lisp/eval_test.exs:806-849
Problem: The Runtime.get/2 and Runtime.get/3 functions handle nil maps (lib/ptc_runner/lisp/runtime.ex:125, 128), but this edge case is not tested. Since this PR extends multi-arity support to get, testing this edge case is in scope.
Fix: Add a test to the "multi-arity builtins" describe block that verifies:

• (get nil :key) returns nil
• (get nil :key "default") returns "default"

Follow the same test pattern as the existing get tests (lines 806-849). The test should go in the "multi-arity builtins" describe block after line 849.

Please fix ALL issues above in a SINGLE commit. Make minimal changes to resolve each issue.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
1	Test for get with nil map	FIX_NOW	Included in fix request above
2	Known get-in/3 nil semantics issue	IGNORE	Already documented in issue #161

Reasoning

Issue 1 - FIX_NOW:

• The PR extends multi-arity support to get, so testing edge cases for get is in scope
• Runtime.get/2 and Runtime.get/3 already handle nil maps (lines 125, 128 in runtime.ex)
• Test is mechanical - follows the same pattern as existing get tests (lines 806-849)
• Only ~10 lines of code needed, no design decisions required

Issue 2 - IGNORE:

• This is a pre-existing semantic issue with get_in/3, not introduced by this PR
• Already documented in issue [From PR #160] Extend multi-arity support to get and get-in #161 under "Edge Cases" section
• Out of scope for a PR focused on multi-arity registration

Status

• FIX_NOW items: 1 (batched in single fix comment above)
• Issues created: 0
• Items ignored: 1

[andreasronge]

Fix Complete

Issue 1: Add test for get with nil map ✅

Changes Made:

• Added test case in the "multi-arity builtins" describe block (after line 849)
• Tests verify:
  • (get nil :key) returns nil
  • (get nil :key "default") returns "default"
• Follows the existing test pattern for get operations

Verification:

• All pre-commit checks passed
• All 971 tests pass

The test documents the edge case behavior already implemented in Runtime.get/2 and Runtime.get/3 (lines 125, 128).

[github-actions]

PR Review: Extend multi-arity support to get and get-in

Summary

Clean, straightforward implementation that extends multi-arity support to get and get-in functions following the pattern established in PR #160. The PR has already been through one review cycle and incorporates the fix for the nil map edge case test.

What's Good

• Follows established pattern: Uses {:multi_arity, {fn/2, fn/3}} tuple structure matching sort-by
• Minimal code change: Only 2 lines changed in env.ex (lines 72-73)
• Comprehensive test coverage: 7 tests covering both arities for get and get-in, arity error cases, and nil map handling
• Strong assertions: Tests verify specific values ("Alice", "default", nil) not just shapes
• Tests organized correctly: New tests in the describe "multi-arity builtins" block alongside sort-by tests
• Edge cases covered: Tests for nil map (added in fix commit), missing keys, and nested paths
• Documentation complete: docs/ptc-lisp-specification.md already documents 2/3-arity signatures (lines 745-748)

Issues (Must Fix)

None.

Suggestions (Optional)

None. The previous review's suggestion (nil map test) was already implemented.

Security

No concerns. The change only exposes existing Runtime.get/2, Runtime.get/3, Runtime.get_in/2, and Runtime.get_in/3 functions that were already implemented and tested.

Documentation

No updates needed. docs/ptc-lisp-specification.md already documents both arities (lines 745-748):

| get | (get m key) | Get value by key |
| get | (get m key default) | Get with default |
| get-in | (get-in m path) | Get nested value |
| get-in | (get-in m path default) | Get nested with default |

Verdict

Approve - The implementation is correct, follows established patterns, has comprehensive tests, and documentation is already complete. The 2-line change in env.ex correctly registers both arities using the multi_arity pattern.

[andreasronge]

Auto-Triage Summary (Cycle 2)

Review Status

The second review cycle found no issues or suggestions. The PR has been approved.

Previous Cycle Actions

The first review cycle identified one improvement:

• Test for get with nil map → Fixed by PR owner via @claude automation

Current Cycle Decisions

#	Issue	Decision	Action
-	No issues found	N/A	PR approved, ready to merge

Status

• FIX_NOW items: 0
• Issues created: 0
• Items ignored: 0

Next Steps

✅ Added ready-to-merge label - auto-merge will handle merging once all checks pass