feat: add json-to-csv conversion via jq#47
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds JSON-to-CSV conversion support using jq, including input validation for file existence and output paths, and updated help documentation. It also introduces new tests for both the conversion feature and the sh log command. Feedback recommends handling empty JSON arrays in the conversion logic to prevent failures, adding a corresponding test case for this edge case, and moving the unrelated sh log tests to a separate pull request to maintain a focused commit history.
| else | ||
| echo -e "${GREEN}✓${NC} cv json to csv dry-run (skipped - jq not installed)" | ||
| ((PASSED++)) | ||
| fi |
There was a problem hiding this comment.
The test coverage for the new feature is great. To make it even more robust, I recommend adding a test case for handling an empty JSON array ([]) as input. This is an important edge case that should be handled gracefully (e.g., by producing an empty output file).
Here's an example test you could add:
# Test: Convert empty JSON array to CSV
echo '[]' > test_cv_empty.json
if command -v jq &>/dev/null; then
u7 cv json test_cv_empty.json to csv yield test_cv_empty.csv >/dev/null 2>&1
if [[ -f "test_cv_empty.csv" && ! -s "test_cv_empty.csv" ]]; then
echo -e "${GREEN}✓${NC} cv json to csv with empty array works"
((PASSED++))
else
echo -e "${RED}✗${NC} cv json to csv with empty array failed"
((FAILED++))
fi
else
echo -e "${GREEN}✓${NC} cv json to csv with empty array (skipped - jq not installed)"
((PASSED++))
fi
Greptile SummaryThis PR adds Remaining feedback (both P2):
Confidence Score: 5/5PR is safe to merge; all remaining findings are minor P2 style and test-quality suggestions that do not affect correctness. The core logic is correct: file-existence guard, atomic write, dry-run, and dependency check are all properly implemented. The jq filter correctly handles empty arrays and produces valid CSV with consistent key ordering. No P0 or P1 issues remain. Both open comments are non-blocking style improvements. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant S as convert.sh
participant JQ as jq
participant FS as Filesystem
U->>S: u7 cv json input.json to csv [yield out.csv]
S->>S: validate "to" keyword
S->>S: resolve output path (default: input.csv)
alt yield with empty arg
S-->>U: Usage error (return 1)
end
S->>FS: check -f input.json
alt file not found
S-->>U: "File not found" (return 1)
end
S->>S: _u7_require jq
alt jq not installed
S-->>U: dependency error (return 1)
end
alt dry-run mode
S-->>U: echo [dry-run] command
else normal run
S->>FS: mktemp output.csv.XXXXXX → tmpfile
S->>JQ: jq -r '...' < input.json > tmpfile
alt jq fails
S->>FS: rm -f tmpfile
S-->>U: return 1
else jq succeeds
S->>FS: mv tmpfile out.csv (atomic)
S-->>U: success (exit 0)
end
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: test.sh
Line: 999
Comment:
**Test only validates data presence, not CSV structure**
The assertion checks that `"Alice"` appears somewhere in the output, but it does not verify that the header row (`"name","age"`) is present and appears _before_ the data rows. A bug that swaps header and data order, or emits only data with no header, would still pass this test.
Consider adding an assertion for the header, e.g.:
```suggestion
assert_contains "cv json to csv works" "Alice" "$result"
assert_contains "cv json to csv has headers" "name" "$result"
```
Or checking that line 1 of the output contains the expected header string.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: lib/convert.sh
Line: 202
Comment:
**Dense one-liner mixes filter logic, I/O, and error handling**
The single line combines a 100+ character `jq` filter, the redirect, the `mv`, and the cleanup fallback all in one expression. This makes it hard to read at a glance and even harder to debug when `jq` emits an error — the failure path (`|| { rm -f "$tmpfile"; return 1; }`) gives the caller no additional context beyond jq's raw stderr.
Consider splitting into two statements with an explicit error check:
```bash
if ! jq -r 'if length == 0 then empty else (.[0] | keys_unsorted) as $keys | $keys, map([.[ $keys[] ]])[] | @csv end' < "$input" > "$tmpfile"; then
rm -f "$tmpfile"
echo "Error: jq failed to convert '$input' to CSV"
return 1
fi
mv "$tmpfile" "$output"
```
This keeps the cleanup and error message clearly separated while also surfacing a human-readable message when jq fails.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix: handle empty JSON arrays in cv json..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces JSON-to-CSV conversion using jq, adds input validation for the conversion process, and includes new tests for both the conversion and the sh log command. Feedback points out a bug when handling empty JSON arrays, providing a fix and suggesting a new test case for this scenario. Additionally, it is recommended to move the unrelated sh log tests to a separate pull request to keep the changes focused.
| else | ||
| local tmpfile | ||
| tmpfile=$(mktemp "${output}.XXXXXX") || { echo "Error: Failed to create temp file"; return 1; } | ||
| jq -r '(.[0] | keys_unsorted) as $keys | $keys, map([.[ $keys[] ]])[] | @csv' < "$input" > "$tmpfile" && mv "$tmpfile" "$output" || { rm -f "$tmpfile"; return 1; } |
There was a problem hiding this comment.
The jq command will fail if the input JSON is an empty array (e.g., []), as .[0] will evaluate to null, causing an error. This will prevent the script from producing a valid (empty) CSV file for a valid (empty) input. To make the conversion more robust, you should handle this edge case.
| jq -r '(.[0] | keys_unsorted) as $keys | $keys, map([.[ $keys[] ]])[] | @csv' < "$input" > "$tmpfile" && mv "$tmpfile" "$output" || { rm -f "$tmpfile"; return 1; } | |
| jq -r 'if length > 0 then (.[0] | keys_unsorted) as $keys | $keys, map([.[ $keys[] ]])[] | @csv else "" end' < "$input" > "$tmpfile" && mv "$tmpfile" "$output" || { rm -f "$tmpfile"; return 1; } |
| # ===== sh log tests ===== | ||
|
|
||
| # Setup: create a temp log file | ||
| LOG_FILE="$TEST_DIR/test.log" | ||
| for i in $(seq 1 30); do | ||
| echo "line $i: log entry" >> "$LOG_FILE" | ||
| done | ||
|
|
||
| # Test: sh log default shows last 20 lines | ||
| result=$(u7 sh log "$LOG_FILE" 2>&1) | ||
| line_count=$(echo "$result" | wc -l | tr -d ' ') | ||
| assert_equals "sh log default shows 20 lines" "20" "$line_count" | ||
| assert_contains "sh log default starts at line 11" "line 11" "$result" | ||
|
|
||
| # Test: sh log with limit | ||
| result=$(u7 sh log "$LOG_FILE" limit 5 2>&1) | ||
| line_count=$(echo "$result" | wc -l | tr -d ' ') | ||
| assert_equals "sh log limit 5 shows 5 lines" "5" "$line_count" | ||
| assert_contains "sh log limit 5 starts at line 26" "line 26" "$result" | ||
|
|
||
| # Test: sh log with match | ||
| echo "ERROR something broke" >> "$LOG_FILE" | ||
| echo "INFO all good" >> "$LOG_FILE" | ||
| echo "ERROR another failure" >> "$LOG_FILE" | ||
| result=$(u7 sh log "$LOG_FILE" match ERROR 2>&1) | ||
| line_count=$(echo "$result" | wc -l | tr -d ' ') | ||
| assert_equals "sh log match finds 2 ERROR lines" "2" "$line_count" | ||
|
|
||
| # Test: sh log missing file | ||
| result=$(u7 sh log nonexistent.log 2>&1) | ||
| assert_contains "sh log reports missing file" "File not found" "$result" | ||
|
|
||
| # Test: sh log no arguments | ||
| result=$(u7 sh log 2>&1) | ||
| assert_contains "sh log no args shows usage" "Usage" "$result" | ||
|
|
||
| # Test: sh log invalid limit | ||
| result=$(u7 sh log "$LOG_FILE" limit abc 2>&1) | ||
| assert_contains "sh log rejects non-integer limit" "limit must be a positive integer" "$result" | ||
|
|
||
| # Test: sh log match without pattern | ||
| result=$(u7 sh log "$LOG_FILE" match 2>&1) | ||
| assert_contains "sh log match without pattern shows usage" "Usage" "$result" | ||
|
|
||
| # Test: sh log unknown subcommand | ||
| result=$(u7 sh log "$LOG_FILE" badarg 2>&1) | ||
| assert_contains "sh log unknown subcommand shows usage" "Usage" "$result" | ||
|
|
There was a problem hiding this comment.
This pull request introduces a significant number of tests for the sh log command. While tests are always welcome, these changes seem unrelated to the primary goal of this PR, which is adding JSON-to-CSV conversion. To maintain a clean and understandable project history, it's best practice to keep pull requests focused on a single, specific feature or bug fix. Please consider moving these sh log tests to a separate pull request.
| else | ||
| echo -e "${GREEN}✓${NC} cv json to csv dry-run (skipped - jq not installed)" | ||
| ((PASSED++)) | ||
| fi |
There was a problem hiding this comment.
The test suite for cv json to csv is missing a case for handling an empty JSON array, like []. The current implementation fails on this input. Adding a test for this scenario would have caught the bug and will prevent future regressions.
You could add a test like this:
# Test: Convert empty JSON array to CSV
echo '[]' > test_empty.json
u7 cv json test_empty.json to csv yield test_empty.csv >/dev/null 2>&1
if [[ -f "test_empty.csv" ]]; then
result=$(cat test_empty.csv)
assert_equals "cv json to csv with empty array is empty" "" "$result"
else
echo -e "${RED}✗${NC} cv json to csv with empty array (file not created)"
((FAILED++))
fi
Summary
csvas a target format foru7 cv json <input> to csv [yield <output>]jqto convert a JSON array of objects to CSV with headers derived fromkeys_unsorted_u7_require jqyaml|csvtarget optionsTest plan
cv json to csv works-- converts JSON array to CSV with correct datacv json to csv reports missing file-- errors on nonexistent inputcv json rejects unsupported format-- returns error for unsupported targetcv json to csv dry-run-- prints dry-run message without writing