fix: address 10 code TODOs across codebase#215
Merged
Conversation
Add character count (-m/--chars), max line length (-L/--max-line-length), and long flag aliases (--bytes, --lines, --words) to the wc builtin. Refactored flag parsing to properly handle short and long flags. Unskipped 5 spec tests: wc_chars_m_flag, wc_max_line_length, wc_long_bytes, wc_bytes_vs_chars, wc_unicode_chars. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
The empty herestring correctly produces a newline (bash behavior). The test was skipped because the spec runner can't represent a single newline as expected output. Rewrote test to verify via follow-up echo. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
Support `VAR = expr` syntax inside arithmetic expansion, e.g. `$((X = X + 1))` evaluates the RHS, assigns to the variable, and returns the value. Correctly distinguishes assignment (=) from comparison (==, !=, <=, >=). Unskipped arith_assign spec test. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
In bash, `x=$(false); echo $?` prints 1 because the exit code of an assignment-only command is the exit code of the last command substitution in its value. Previously bashkit always returned 0 for assignment-only commands. Two changes: 1. Set last_exit_code during command substitution word expansion 2. Use last_exit_code as return code for assignment-only commands Unskipped subst_exit_code spec test. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
Implement sort -f (fold case) which compares lines case-insensitively while preserving original casing in output. Uses to_lowercase() for comparison keys. Unskipped sort_case_insensitive spec test. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
The uniq -d (only duplicates) and -u (only unique) flags were already implemented in the Rust code with passing unit tests, but the spec tests still had stale skip markers. Removed the skip directives and verified both spec tests pass. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
Replace TODO comments with proper documentation explaining design decisions: 1. Background execution (&): runs synchronously in virtual mode since OS process spawning is excluded from the sandbox by design. 2. Shell options (-e, -x, -v, -u, etc.): documented why each option is accepted but not enforced, with specific notes on what implementation would require. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
The commented-out [[licenses.clarify]] template with a TODO was a placeholder that's never been needed. Replace with a clear comment explaining when clarifications would be added. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
Update spec_tests.rs total from 87 to 76 skipped tests (11 unskipped in this branch). Fix clippy unnecessary_sort_by lint in sort -f. https://claude.ai/code/session_016hW1DMEWM7SnaK1n6fZtxk
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
2 participants
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
Addresses 10 TODO/skipped-test items across the codebase, reducing skipped spec tests from 87 to 76 (11 unskipped) and resolving all explicit TODO comments in source code.
Changes by commit:
feat(wc): add -m, -L, --bytes/--lines/--words/--chars flags — Full wc improvement: character count (-m/--chars), max line length (-L), and long flag aliases. Unskips 5 spec tests.
fix(tests): unskip herestring_empty test — The empty herestring correctly produces a newline (bash behavior). Rewrote test to verify via follow-up echo since the spec runner can't represent a bare newline as expected output.
feat(interpreter): implement arithmetic assignment in $(()) — Support
VAR = exprinside arithmetic expansion (e.g.$((X = X + 1))). Correctly distinguishes from==/!=/<=/>=. Unskips arith_assign.fix(interpreter): propagate exit code from command substitution —
x=$(false); echo $?now correctly prints 1. Sets last_exit_code during substitution and preserves it through assignment-only commands.feat(sort): add -f flag for case-insensitive sorting — Implements fold-case comparison via
sort_by_key. Unskips sort_case_insensitive.fix(tests): unskip uniq -d/-u spec tests — These flags were already implemented in code with passing unit tests but had stale skip markers in spec tests.
chore(interpreter): resolve background execution and shell options TODOs — Replace TODO comments with proper documentation explaining design decisions (background runs synchronously in sandbox; shell options accepted but not enforced).
chore(deny): replace license clarification TODO — Remove unused template placeholder with explanatory comment.
Test plan
cargo fmt --checkpassescargo clippy --all-targets --all-features -- -D warningspassescargo test --all-features— all 977+ tests pass, 0 failures