Improve response formatting

[UltraInstinct21]

Fixes: #54

Improves response formatting by adding multi-line explanations, numbered warnings, and clearer overall structure.

[benodiwal]

/claude

[github-actions]

Claude finished @benodiwal's task in 1m 12s —— View job

---

Code Review

Tasks

• Read and analyze the changed files
• Review code quality and correctness
• Provide comprehensive feedback
• Final summary

---

Summary

This PR improves the formatting of plain text responses by adding multi-line text wrapping, numbered warnings, and clearer structure. The changes are well-implemented and the test suite has been updated appropriately.

Changes Overview

Added functionality:

• New format_multiline_comment() function for word-wrapping long text to fit within SQL comment lines
• Enhanced plain text output with clearer section headers
• Numbered warnings for multiple warning messages
• Consistent multi-line formatting for explanations, warnings, and visualizations

Files modified:

• src/core/response_formatter.cpp: Core formatting improvements (+37/-6)
• tests/unit/test_response_formatter.cpp: Updated test to match new format (+1/-1)

---

Code Quality Assessment

✅ Strengths

1. Good Design: The format_multiline_comment() helper function is well-designed with configurable prefix and width parameters

2. Comprehensive Testing: The test suite thoroughly covers various scenarios (single warning, multiple warnings, JSON output, plain text, etc.)

3. Backward Compatible: JSON output remains unchanged, only plain text formatting improved

4. Clear Intent: The formatting changes make the output more readable with proper section headers like -- Query:, -- Explanation:, etc.

5. Edge Cases Handled: The function properly handles empty lines and prevents adding trailing content when only prefix exists

⚠️ Issues & Suggestions

1. Default Parameter in .cpp File (Minor Issue - src/core/response_formatter.cpp:9)

   std::string format_multiline_comment(const std::string& text,
                                        const std::string& prefix = "--   ",
                                        int max_width = 70) {

   Default parameters should be specified in the function declaration (header file) or at the point of first declaration, not in the implementation file. Since this is a file-local helper function, consider either:

   • Making it a static function in an anonymous namespace
   • Moving it to the header if it might be reused elsewhere
   • Removing default parameters from the implementation (they only belong in declarations)

2. Missing Const Correctness (src/core/response_formatter.cpp:9)
   The max_width parameter could be const int or better yet, std::size_t since it represents a size/width value that should never be negative.

3. Potential Off-by-One in Width Calculation (src/core/response_formatter.cpp:17)

   if (current_line.length() + word.length() + 1 > max_width) {

   The + 1 accounts for the space, but this check happens before adding the space. Consider whether the logic correctly handles the boundary case when current_line.length() + word.length() + 1 == max_width.

4. Missing Header Include (src/core/response_formatter.cpp:8)
   The function uses std::istringstream which is included via <sstream> on line 3, but it's good practice to explicitly include <string> for std::string operations.

5. Inconsistent Spacing in Warnings (src/core/response_formatter.cpp:120-122)

   if (i + 1 < warnings.size()) {
     output << "\n";
   }

   When multiple warnings are present, a newline is added between them, but the format_multiline_comment() function doesn't add a trailing newline. This could lead to inconsistent spacing. Verify the actual output looks correct.

6. No Tests for Word Wrapping (tests/unit/test_response_formatter.cpp)
   While the tests verify that multi-line content is present, there are no tests that specifically verify the word-wrapping behavior of format_multiline_comment(). Consider adding a test with a very long explanation (>70 chars) to ensure wrapping works correctly.

---

Recommendations

High Priority:

• Fix the default parameters issue (move to declaration or use anonymous namespace)

Medium Priority:

• Add unit tests specifically for the format_multiline_comment() function with long text (>70 chars) to verify wrapping behavior
• Consider making format_multiline_comment() a private static method of ResponseFormatter for better encapsulation

Low Priority:

• Use std::size_t instead of int for max_width parameter
• Verify the spacing behavior with multiple warnings produces the expected output

---

Test Coverage

The test suite is comprehensive with 22 test cases covering:

• ✅ Plain text formatting (basic, with explanation, with warnings, with visualization)
• ✅ JSON formatting (all variations)
• ✅ Single vs multiple warnings
• ✅ Edge cases (empty strings, false flags)
• ✅ Pretty-printing verification

Missing test coverage:

• ⚠️ Long text that needs word wrapping (>70 characters)
• ⚠️ Text with newlines or special characters

---

Overall Assessment

Grade: B+ (Good with minor issues)

The implementation successfully achieves its goal of improving response formatting. The code is readable, well-tested, and the changes are focused. The main issues are minor style/best practice concerns that don't affect functionality but should be addressed for code quality.

Recommendation: ✅ Approve with suggestions - The PR can be merged as-is, but addressing the default parameter issue and adding word-wrapping tests would improve code quality.

---

[UltraInstinct21]

Updates

• moved format_multiline_comment into an anonymous namespace
• Improved wrapping logic and spacing for multi-line explanations and warnings
• Switched max_width to std::size_t for better type safety

I haven’t added unit tests for format_multiline_comment() yet. If I can get some guidance on it, it would be great!
@benodiwal