navicore/seq-lisp

Fork 0

⏺ All tests pass (8 Seq, 449 Lisp, 6 LSP). #57

Merged

navicore merged 2 commits from strings into main

2026-02-02 05:07:47 +00:00

navicore commented

2026-02-02 04:53:41 +00:00

(Migrated from github.com)

Summary of completed work:

Added 8 new Lisp string operations as wrappers around Seq's string builtins:
┌─────────────────────┬──────────────────────────┬────────────────────────────────────────────┐
│ Function │ Description │ Example │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-split │ Split by delimiter │ (string-split "a,b,c" ",") → ("a" "b" "c") │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-trim │ Remove whitespace │ (string-trim " hi ") → "hi" │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-upcase │ Uppercase │ (string-upcase "hello") → "HELLO" │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-downcase │ Lowercase │ (string-downcase "HELLO") → "hello" │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-contains? │ Check substring │ (string-contains? "hello" "ell") → #t │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-starts-with? │ Check prefix │ (string-starts-with? "hello" "he") → #t │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-find │ Find index │ (string-find "hello" "ll") → 2 │
├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤
│ string-chomp │ Remove trailing newlines │ (string-chomp "hi\n") → "hi" │
└─────────────────────┴──────────────────────────┴────────────────────────────────────────────┘
These join the existing string-length, substring, and string-append to provide a complete string manipulation toolkit. The roadmap has been updated to
reflect this completion.

Summary of completed work: Added 8 new Lisp string operations as wrappers around Seq's string builtins: ┌─────────────────────┬──────────────────────────┬────────────────────────────────────────────┐ │ Function │ Description │ Example │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-split │ Split by delimiter │ (string-split "a,b,c" ",") → ("a" "b" "c") │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-trim │ Remove whitespace │ (string-trim " hi ") → "hi" │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-upcase │ Uppercase │ (string-upcase "hello") → "HELLO" │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-downcase │ Lowercase │ (string-downcase "HELLO") → "hello" │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-contains? │ Check substring │ (string-contains? "hello" "ell") → #t │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-starts-with? │ Check prefix │ (string-starts-with? "hello" "he") → #t │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-find │ Find index │ (string-find "hello" "ll") → 2 │ ├─────────────────────┼──────────────────────────┼────────────────────────────────────────────┤ │ string-chomp │ Remove trailing newlines │ (string-chomp "hi\n") → "hi" │ └─────────────────────┴──────────────────────────┴────────────────────────────────────────────┘ These join the existing string-length, substring, and string-append to provide a complete string manipulation toolkit. The roadmap has been updated to reflect this completion.

claude[bot] commented

2026-02-02 04:55:20 +00:00

(Migrated from github.com)

PR Review: String Operations Implementation

Summary

This PR adds 8 new string manipulation functions as wrappers around Seq's string builtins. All tests pass (8 Seq, 449 Lisp, 6 LSP), and the implementation follows the established patterns in the codebase.

Code Quality ✓

Strengths:

Consistent implementation pattern: All new functions follow the same structure as existing string operations (string-length, substring, string-append)
Proper arity checking: Each function validates argument count using check-arity
Type validation: All functions properly check that arguments are strings and provide clear error messages
Stack-based discipline: The Seq stack-oriented code follows consistent patterns with clear stack comments
Error propagation: Errors are properly propagated through the evaluation chain using eval-err? checks

Code style observations:

The implementation is well-commented with stack effect annotations (e.g., # Stack: Acc SeqList Idx)
Function naming follows Lisp conventions with hyphens and ? for predicates
Dispatcher updated correctly with all new functions added to the chain

Potential Issues & Considerations

1. Missing Test Coverage ⚠️

Critical: No tests found for any of the 8 new functions in tests/lisp/core/strings.slisp.

The existing test file only covers:

string-length
substring
string-append

Recommendation: Add comprehensive tests for:

;; string-split
(test 'string-split-basic (assert-eq '("a" "b" "c") (string-split "a,b,c" ",")))
(test 'string-split-empty (assert-eq '("") (string-split "" ",")))
(test 'string-split-no-delim (assert-eq '("hello") (string-split "hello" ",")))

;; string-trim
(test 'string-trim-both (assert-eq "hello" (string-trim "  hello  ")))
(test 'string-trim-none (assert-eq "hello" (string-trim "hello")))

;; string-upcase / string-downcase
(test 'string-upcase-basic (assert-eq "HELLO" (string-upcase "hello")))
(test 'string-downcase-basic (assert-eq "hello" (string-downcase "HELLO")))

;; string-contains?
(test 'string-contains-yes (assert-true (string-contains? "hello" "ell")))
(test 'string-contains-no (assert-false (string-contains? "hello" "xyz")))

;; string-starts-with?
(test 'string-starts-with-yes (assert-true (string-starts-with? "hello" "he")))
(test 'string-starts-with-no (assert-false (string-starts-with? "hello" "lo")))

;; string-find
(test 'string-find-exists (assert-eq 2 (string-find "hello" "ll")))
(test 'string-find-missing (assert-eq -1 (string-find "hello" "xyz")))

;; string-chomp
(test 'string-chomp-newline (assert-eq "hello" (string-chomp "hello\n")))
(test 'string-chomp-none (assert-eq "hello" (string-chomp "hello")))

2. Edge Case Handling

Empty string delimiter in string-split: src/eval.seq:2099
The Seq string.split behavior with empty delimiter should be documented or tested. Does it split into individual characters or error?

Unicode handling:
All functions rely on Seq's built-in string methods. Assuming Seq handles Unicode correctly, but worth verifying for:

string-upcase / string-downcase with non-ASCII characters
string-length with multi-byte characters

3. Documentation

Function signatures in comments: The implementation has good function headers but could benefit from examples in the comments:

# (string-split str delimiter) - split string by delimiter, returns list of strings
# Example: (string-split "a,b,c" ",") => ("a" "b" "c")

Performance Considerations

✓ No concerns: All functions are thin wrappers around Seq's efficient built-in string operations. The stack-based evaluation is consistent with the rest of the codebase.

The string-list-to-sexpr-list helper at src/eval.seq:2109-2123 uses recursion efficiently and includes a list-reverse operation which is appropriate for building lists in stack order.

Security Concerns

✓ No issues identified:

No unsafe operations or potential injection vectors
Type checking prevents non-string arguments
Relying on Seq's built-in string operations which should be safe

Roadmap Update ✓

The ROADMAP.md update is accurate and properly marks the string operations as completed. The table in the PR description provides clear documentation.

Overall Assessment

Score: 8/10

This is solid, production-ready code that follows established patterns. The only significant gap is test coverage for the new functions.

Recommendation: Merge after adding tests for the 8 new functions. The implementation is sound, but tests are essential to prevent regressions and document expected behavior.

Nice work on:

Clean, consistent implementation
Comprehensive set of string utilities
Maintaining backward compatibility
Clear roadmap updates

Action items:

Add tests to tests/lisp/core/strings.slisp for all 8 new functions
Consider documenting edge cases (empty delimiter, Unicode)
Verify CI passes with new tests

Once tests are added, this PR is ready to merge! 🚀

# PR Review: String Operations Implementation ## Summary This PR adds 8 new string manipulation functions as wrappers around Seq's string builtins. All tests pass (8 Seq, 449 Lisp, 6 LSP), and the implementation follows the established patterns in the codebase. ## Code Quality ✓ **Strengths:** - **Consistent implementation pattern**: All new functions follow the same structure as existing string operations (`string-length`, `substring`, `string-append`) - **Proper arity checking**: Each function validates argument count using `check-arity` - **Type validation**: All functions properly check that arguments are strings and provide clear error messages - **Stack-based discipline**: The Seq stack-oriented code follows consistent patterns with clear stack comments - **Error propagation**: Errors are properly propagated through the evaluation chain using `eval-err?` checks **Code style observations:** - The implementation is well-commented with stack effect annotations (e.g., `# Stack: Acc SeqList Idx`) - Function naming follows Lisp conventions with hyphens and `?` for predicates - Dispatcher updated correctly with all new functions added to the chain ## Potential Issues & Considerations ### 1. Missing Test Coverage ⚠️ **Critical:** No tests found for any of the 8 new functions in `tests/lisp/core/strings.slisp`. The existing test file only covers: - `string-length` - `substring` - `string-append` **Recommendation:** Add comprehensive tests for: ```lisp ;; string-split (test 'string-split-basic (assert-eq '("a" "b" "c") (string-split "a,b,c" ","))) (test 'string-split-empty (assert-eq '("") (string-split "" ","))) (test 'string-split-no-delim (assert-eq '("hello") (string-split "hello" ","))) ;; string-trim (test 'string-trim-both (assert-eq "hello" (string-trim " hello "))) (test 'string-trim-none (assert-eq "hello" (string-trim "hello"))) ;; string-upcase / string-downcase (test 'string-upcase-basic (assert-eq "HELLO" (string-upcase "hello"))) (test 'string-downcase-basic (assert-eq "hello" (string-downcase "HELLO"))) ;; string-contains? (test 'string-contains-yes (assert-true (string-contains? "hello" "ell"))) (test 'string-contains-no (assert-false (string-contains? "hello" "xyz"))) ;; string-starts-with? (test 'string-starts-with-yes (assert-true (string-starts-with? "hello" "he"))) (test 'string-starts-with-no (assert-false (string-starts-with? "hello" "lo"))) ;; string-find (test 'string-find-exists (assert-eq 2 (string-find "hello" "ll"))) (test 'string-find-missing (assert-eq -1 (string-find "hello" "xyz"))) ;; string-chomp (test 'string-chomp-newline (assert-eq "hello" (string-chomp "hello\n"))) (test 'string-chomp-none (assert-eq "hello" (string-chomp "hello"))) ``` ### 2. Edge Case Handling **Empty string delimiter in `string-split`:** src/eval.seq:2099 The Seq `string.split` behavior with empty delimiter should be documented or tested. Does it split into individual characters or error? **Unicode handling:** All functions rely on Seq's built-in string methods. Assuming Seq handles Unicode correctly, but worth verifying for: - `string-upcase` / `string-downcase` with non-ASCII characters - `string-length` with multi-byte characters ### 3. Documentation **Function signatures in comments:** The implementation has good function headers but could benefit from examples in the comments: ```seq # (string-split str delimiter) - split string by delimiter, returns list of strings # Example: (string-split "a,b,c" ",") => ("a" "b" "c") ``` ## Performance Considerations ✓ **No concerns:** All functions are thin wrappers around Seq's efficient built-in string operations. The stack-based evaluation is consistent with the rest of the codebase. The `string-list-to-sexpr-list` helper at src/eval.seq:2109-2123 uses recursion efficiently and includes a `list-reverse` operation which is appropriate for building lists in stack order. ## Security Concerns ✓ **No issues identified:** - No unsafe operations or potential injection vectors - Type checking prevents non-string arguments - Relying on Seq's built-in string operations which should be safe ## Roadmap Update ✓ The ROADMAP.md update is accurate and properly marks the string operations as completed. The table in the PR description provides clear documentation. ## Overall Assessment **Score: 8/10** This is solid, production-ready code that follows established patterns. The only significant gap is test coverage for the new functions. **Recommendation:** Merge after adding tests for the 8 new functions. The implementation is sound, but tests are essential to prevent regressions and document expected behavior. --- **Nice work on:** - Clean, consistent implementation - Comprehensive set of string utilities - Maintaining backward compatibility - Clear roadmap updates **Action items:** 1. Add tests to `tests/lisp/core/strings.slisp` for all 8 new functions 2. Consider documenting edge cases (empty delimiter, Unicode) 3. Verify CI passes with new tests Once tests are added, this PR is ready to merge! 🚀

claude[bot] commented

2026-02-02 05:01:46 +00:00

(Migrated from github.com)

PR Review: String Operations Implementation

Overview

This PR adds 8 new string manipulation functions to SeqLisp, wrapping Seq's built-in string operations. The implementation is well-structured with comprehensive test coverage (40 new tests).

✅ Strengths

Code Quality

Consistent patterns: All new functions follow the established conventions in eval.seq for:
- Arity checking with descriptive error messages
- Argument evaluation and type validation
- Error propagation through the EvalResult union
- Stack manipulation with clear comments
Integration: Properly integrated into the dispatch chain, builtin list, and function enumeration
Documentation: Roadmap updated with clear examples and completion status

Test Coverage

The test suite is excellent with 40 well-designed tests covering:

Basic functionality for each operation
Edge cases (empty strings, no matches, boundary conditions)
Error conditions (type checking is implicit in Lisp tests)
All 8 new functions comprehensively tested

Implementation Details

string-split: Correctly converts Seq List to SexprList with proper recursion
Type checking: Consistent validation of string arguments before operations
Boolean returns: Proper use of #t/#f symbols for predicate functions
Error messages: Clear, descriptive errors identifying function name and issue

🔍 Observations

Minor Naming Inconsistency (Non-blocking)

Line 2228 in src/eval.seq:

: eval-string-starts-with-env ( SexprList Env -- EvalResult )

Should be eval-string-starts-with**-with**-env to match the naming pattern of all other functions (e.g., eval-string-contains-with-env on line 2189).

Impact: Low - the function is correctly called in the dispatch (line 963 uses the correct name eval-string-starts-with-env), so this is purely a naming consistency issue. The code works correctly as-is.

Design Considerations

string-find return value: Returns -1 for not found (C-style). This is reasonable and consistent with many languages, though some Lisps use nil. The current choice is fine and well-documented in tests.
string-chomp behavior: The test on line 117 shows (string-chomp "hello\n\n") returns "hello\n" (removes only the last newline). This matches Ruby's chomp semantics, which is intentional and correct.
Stack operations: The implementation uses manual stack manipulation (rot, swap, dup, nip, etc.) which is idiomatic for Seq. The inline comments help track stack states.

🎯 Performance

No performance concerns:

Operations delegate to native Seq string builtins
List conversion in string-split uses tail recursion
No unnecessary allocations or repeated evaluations

🔒 Security

No security concerns:

No string interpolation or code execution
Type checking prevents injection issues
All operations are pure functions

📊 Testing

Excellent coverage across all dimensions:

string-upcase:       4 tests
string-downcase:     4 tests  
string-trim:         6 tests
string-split:        5 tests
string-contains?:    6 tests
string-starts-with?: 5 tests
string-find:         5 tests
string-chomp:        5 tests

Edge cases well-covered: empty strings, no matches, boundary conditions, multi-character delimiters.

✅ Recommendation

APPROVE - This is high-quality work that:

Follows established patterns perfectly
Has comprehensive test coverage
Properly integrates with the existing codebase
Includes clear documentation

The minor naming inconsistency noted above is cosmetic and doesn't affect functionality. The PR can be merged as-is, or the naming can be corrected in a follow-up commit if desired.

📝 Optional Follow-up

If you want to address the naming consistency:

# Line 2228
-: eval-string-starts-with-env ( SexprList Env -- EvalResult )
+: eval-string-starts-with-with-env ( SexprList Env -- EvalResult )

# Line 963 (dispatch)
-    drop eval-string-starts-with-env
+    drop eval-string-starts-with-with-env

But again, this is purely cosmetic and non-blocking.

Great work! 🎉

# PR Review: String Operations Implementation ## Overview This PR adds 8 new string manipulation functions to SeqLisp, wrapping Seq's built-in string operations. The implementation is well-structured with comprehensive test coverage (40 new tests). ## ✅ Strengths ### Code Quality - **Consistent patterns**: All new functions follow the established conventions in `eval.seq` for: - Arity checking with descriptive error messages - Argument evaluation and type validation - Error propagation through the `EvalResult` union - Stack manipulation with clear comments - **Integration**: Properly integrated into the dispatch chain, builtin list, and function enumeration - **Documentation**: Roadmap updated with clear examples and completion status ### Test Coverage The test suite is excellent with 40 well-designed tests covering: - Basic functionality for each operation - Edge cases (empty strings, no matches, boundary conditions) - Error conditions (type checking is implicit in Lisp tests) - All 8 new functions comprehensively tested ### Implementation Details - **string-split**: Correctly converts Seq List to SexprList with proper recursion - **Type checking**: Consistent validation of string arguments before operations - **Boolean returns**: Proper use of `#t`/`#f` symbols for predicate functions - **Error messages**: Clear, descriptive errors identifying function name and issue ## 🔍 Observations ### Minor Naming Inconsistency (Non-blocking) Line 2228 in `src/eval.seq`: ```seq : eval-string-starts-with-env ( SexprList Env -- EvalResult ) ``` Should be `eval-string-starts-with**-with**-env` to match the naming pattern of all other functions (e.g., `eval-string-contains-with-env` on line 2189). **Impact**: Low - the function is correctly called in the dispatch (line 963 uses the correct name `eval-string-starts-with-env`), so this is purely a naming consistency issue. The code works correctly as-is. ### Design Considerations 1. **string-find return value**: Returns `-1` for not found (C-style). This is reasonable and consistent with many languages, though some Lisps use `nil`. The current choice is fine and well-documented in tests. 2. **string-chomp behavior**: The test on line 117 shows `(string-chomp "hello\n\n")` returns `"hello\n"` (removes only the last newline). This matches Ruby's `chomp` semantics, which is intentional and correct. 3. **Stack operations**: The implementation uses manual stack manipulation (`rot`, `swap`, `dup`, `nip`, etc.) which is idiomatic for Seq. The inline comments help track stack states. ## 🎯 Performance No performance concerns: - Operations delegate to native Seq string builtins - List conversion in `string-split` uses tail recursion - No unnecessary allocations or repeated evaluations ## 🔒 Security No security concerns: - No string interpolation or code execution - Type checking prevents injection issues - All operations are pure functions ## 📊 Testing Excellent coverage across all dimensions: ``` string-upcase: 4 tests string-downcase: 4 tests string-trim: 6 tests string-split: 5 tests string-contains?: 6 tests string-starts-with?: 5 tests string-find: 5 tests string-chomp: 5 tests ``` Edge cases well-covered: empty strings, no matches, boundary conditions, multi-character delimiters. ## ✅ Recommendation **APPROVE** - This is high-quality work that: - Follows established patterns perfectly - Has comprehensive test coverage - Properly integrates with the existing codebase - Includes clear documentation The minor naming inconsistency noted above is cosmetic and doesn't affect functionality. The PR can be merged as-is, or the naming can be corrected in a follow-up commit if desired. ## 📝 Optional Follow-up If you want to address the naming consistency: ```seq # Line 2228 -: eval-string-starts-with-env ( SexprList Env -- EvalResult ) +: eval-string-starts-with-with-env ( SexprList Env -- EvalResult ) # Line 963 (dispatch) - drop eval-string-starts-with-env + drop eval-string-starts-with-with-env ``` But again, this is purely cosmetic and non-blocking. Great work! 🎉