-
Notifications
You must be signed in to change notification settings - Fork 4
feat(client): implement client class and improve TUI along with tests #54
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 a new "cd" command to the client interface, implements port number input and validation, and removes unused legacy server handler functionality to improve overall TUI and client behavior.
- Adds support for the "cd" command and related directory updates.
- Introduces a new get_port_number() method with port validation.
- Removes the ServerHandler interface and related member from ConnectionManager.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/client/interface.cpp | Adds "cd" command to the help and validation list; introduces port input. |
| src/client/connection_manager.cpp | Removes legacy ServerHandler-related code. |
| src/client/client.cpp | Updates client behavior and logging around connection process. |
| include/client/interface.hpp | Refactors TUI to implement ITUI and includes updated documentation. |
| include/client/connection_manager.hpp | Removes the ServerHandler declaration and updates ServerInfo struct. |
| include/client/client.hpp | Updates client class declarations and dependencies. |
Files not reviewed (1)
- tests/unittests/client/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (2)
src/client/interface.cpp:95
- Consider adding unit tests to cover the port number validation logic, including cases for empty, non-numeric, and out-of-range inputs.
if (port.empty() || !std::all_of(port.begin(), port.end(), ::isdigit) || std::stoi(port) < 1 || std::stoi(port) > 65535) {
src/client/interface.cpp:43
- Ensure that the new 'cd' command and its associated current directory update are fully covered by tests to verify correct behavior on success.
{"cd", "Change the current directory (cd <directory>)"},
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 a new client class with an improved TUI and extended test coverage while refactoring connection management and interface definitions. Key changes include the addition of a "cd" command in the TUI, implementation of a port number input with validation, and removal of the obsolete ServerHandler interface.
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/client/interface.cpp | Added "cd" command entry and implemented get_port_number() with port validation. |
| src/client/connection_manager.cpp | Removed the unused ServerHandler setter and related member variable to simplify connection logic. |
| src/client/client.cpp | Implemented the client’s connection workflow and TUI interactions with added logging. |
| include/client/interface.hpp | Updated interface definitions with the new ITUI abstraction. |
| include/client/connection_manager.hpp | Refactored ServerInfo and removed the ServerHandler interface to streamline connection management. |
| include/client/client.hpp | Expanded the Client class header to support dependency injection for testing and modularization. |
Files not reviewed (1)
- tests/unittests/client/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (1)
src/client/interface.cpp:95
- [nitpick] Consider caching the result of std::stoi(port) in a local variable to avoid duplicating the conversion call, which improves both performance and consistency.
if (port.empty() || !std::all_of(port.begin(), port.end(), ::isdigit) || std::stoi(port) < 1 || std::stoi(port) > 65535) {
|
Can you please also implement the main.cpp file on the client side, and modify the cmake to make an executable from this? |
…rride methods in TUI class Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
… for cleaner interface Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
…ommand processing Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
…or compatibility Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
… and logging configuration Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
|
Why am I asked to enter server-ip even after I provide it using command line argument? |
|
Also, can you please remove the redundant semicolons? |
Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
…tup and connection management Signed-off-by: vibhatsu <maulikbarot2915@gmail.com>
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 implements a new client class along with improvements in the terminal user interface and logging configuration, while also enhancing the connection manager and adding relevant tests. Key changes include:
- Adding a new function to configure logging based on command line arguments.
- Implementing a client class with robust error handling and connection management.
- Extending the TUI to support the new "cd" command and port input.
Reviewed Changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/logging.cpp | Added configure_logging() to set up logging via command line args. |
| src/client/main.cpp | Updated main() to use argparse for argument parsing and error handling. |
| src/client/interface.cpp | Extended TUI with a "cd" command and added server port number input. |
| src/client/connection_manager.cpp | Enhanced connection manager with connection info setters, getters, and reset logic. |
| src/client/client.cpp | Implemented the core client functionality with improved exception handling. |
| include/common/logging.hpp | Declared the new configure_logging() function. |
| include/client/interface.hpp | Introduced ITUI interface and updated TUI to inherit from it. |
| include/client/connection_manager.hpp | Updated the connection manager interface and removed unused server handler reference. |
| include/client/client.hpp | Updated client interface to reflect new connection and TUI usage. |
Files not reviewed (4)
- .gitmodules: Language not supported
- CMakeLists.txt: Language not supported
- src/client/CMakeLists.txt: Language not supported
- tests/unittests/client/CMakeLists.txt: Language not supported
closes #40
closes #9