-
Notifications
You must be signed in to change notification settings - Fork 4
feat(colors): add colorized output for user prompts and responses #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds colorized output and improves logging/configuration throughout the codebase while also updating client–server interaction interfaces.
- Updates the client and server modules to use new color helper functions for user prompts and responses.
- Refactors client handler interfaces and associated tests to support the new API signature.
- Enhances logging and argument parsing in both client and server main functions.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/server/server_connection_manager_test.cpp | Updated mock client handler and test client connection handling. |
| src/server/server.cpp | Added logging and updated server initialization for colorized output. |
| src/server/main.cpp | Redesigned argument parsing and signal handling with improved logging. |
| src/server/connection_manager.cpp | Refactored client handler usage and connection handling logic. |
| src/server/client_info.cpp | Added file system tree support with proper mutex protection. |
| src/server/CMakeLists.txt | Updated build configuration to include new client_info. |
| src/common/logging.cpp | Updated configure_logging to accept a logger name. |
| src/client/response_manager.cpp | Integrated colorized responses and additional debug logging. |
| src/client/main.cpp | Updated logging initialization in the client main. |
| src/client/interface.cpp | Modified prompt output to include colors and improve UX. |
| src/client/connection_manager.cpp | Added debug logging for received encrypted data. |
| src/client/client.cpp | Updated response handling and introduced a debug print. |
| include/server/server.hpp | Added documentation and adjusted member types for logging. |
| include/server/request_manager.hpp | Updated client handler interface and added colorized docs. |
| include/server/connection_manager.hpp | Switched from ClientHandler to IClientHandler for consistency. |
| include/server/client_info.hpp | Introduced file system tree and enhanced ClientInfo structure. |
| include/common/logging.hpp | Updated logging API declarations. |
| include/client/response_manager.hpp | Added an explicit constructor for logger name support. |
| include/client/colors.hpp | New header defining ANSI color codes and helper functions. |
Comments suppressed due to low confidence (1)
src/client/client.cpp:134
- The debug print statement left in production code may be unintended; consider removing it or replacing it with a proper logging call.
std::cout << "response success" << std::endl;
Signed-off-by: Praneeth Sarode <praneethsarode@gmail.com>
…for enabling/disabling colors Signed-off-by: Praneeth Sarode <praneethsarode@gmail.com>
…formatting Signed-off-by: Praneeth Sarode <praneethsarode@gmail.com>
…formatting Signed-off-by: Praneeth Sarode <praneethsarode@gmail.com>
24a55b0 to
e6bc0aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces colorized output for user prompts and responses, enhancing the visual feedback both in the CLI and in test logs. Key changes include:
- Integrating ANSI color codes into output messages (success, error, info, warnings) in both client and response manager.
- Updating unit and integration tests to disable colors for reliable text comparisons.
- Adding new colors support files (colors.hpp and colors.cpp) and updating CMake files accordingly.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/client/client_response_manager_test.cpp | Added color disable/enable in test fixtures for plain text. |
| tests/unittests/client/client_integration_test.cpp | Updated tests to include colors; adjusted output assertions. |
| src/client/response_manager.cpp | Introduced colorized formatting in responses and helper outputs. |
| src/client/interface.cpp | Updated CLI prompts and output messages with color codes. |
| src/client/client.cpp | Modified connection success message to include colors. |
| src/client/colors.cpp & include/client/colors.hpp | New files providing ANSI color escape code wrappers. |
| src/client/CMakeLists.txt | Updated to compile the new colors.cpp file. |
| size_str = size_stream.str(); | ||
|
|
||
| // Apply appropriate color based on file size if colors are enabled | ||
| if (colors::use_colors) { |
Copilot
AI
May 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The file size formatting logic with color codes is repeated in several branches; consider extracting this pattern into a helper function to reduce duplication and improve readability.
| size_t pos = desc.find('('); | ||
| if (pos != std::string::npos && | ||
| desc.find(')', pos) != std::string::npos) { | ||
| arg_part = desc.substr(pos); | ||
| desc = desc.substr(0, pos - 1); // -1 to remove trailing space |
Copilot
AI
May 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The approach for splitting command descriptions into the base text and argument parts feels a bit complex and could be made more robust; consider refactoring this logic to better handle unexpected formatting.
| size_t pos = desc.find('('); | |
| if (pos != std::string::npos && | |
| desc.find(')', pos) != std::string::npos) { | |
| arg_part = desc.substr(pos); | |
| desc = desc.substr(0, pos - 1); // -1 to remove trailing space | |
| std::regex desc_regex(R"((.*?)(\s*\(.*\))?)"); | |
| std::smatch match; | |
| if (std::regex_match(desc, match, desc_regex)) { | |
| desc = match[1].str(); // Base text | |
| arg_part = match[2].str(); // Argument part (if any) |
No description provided.