feat: Improve command line argument handling #56
Conversation
WalkthroughThe pull request modifies the argument parsing functions in both Changes
Assessment against linked issues
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/spider/scheduler/scheduler.cpp (1)
41-44: Consider using std::format for usage message.The usage message could benefit from using
std::formatfor better maintainability, consistent with the project's use of fmt library elsewhere.-char const* const cUsage - = "Usage: spider_scheduler --host <host> --port <port> --storage_url <url>"; +char const* const cUsage = std::format( + "Usage: {} --host <host> --port <port> --storage_url <url>", + "spider_scheduler");src/spider/worker/worker.cpp (1)
53-55: Consider using std::format for usage message.The usage message could benefit from using
std::formatfor better maintainability, consistent with the project's use of fmt library elsewhere.-char const* const cUsage = "Usage: spider_worker --host <host> --storage_url <storage_url> --libs <libs>"; +char const* const cUsage = std::format( + "Usage: {} --host <host> --storage_url <storage_url> --libs <libs>", + "spider_worker");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/spider/scheduler/scheduler.cpp(3 hunks)src/spider/worker/worker.cpp(3 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spider/worker/worker.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
🔇 Additional comments (6)
src/spider/scheduler/scheduler.cpp (3)
5-5: LGTM!The addition of the
<iostream>header is necessary for the new error output functionality.
172-172: LGTM!The main function correctly uses the new
parse_argssignature.
45-96: Verify error handling for edge cases.The argument parsing logic looks good, but let's verify the handling of edge cases:
- When only
--helpis provided- When invalid port numbers are supplied
✅ Verification successful
Edge cases are properly handled in the argument parsing implementation
The code demonstrates robust handling of all key edge cases:
- Help flag (--help) displays usage information and available options
- Port validation is enforced through the unsigned short type
- Missing or invalid arguments show clear error messages with usage instructions
- Distinct error codes for different failure scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering argument parsing edge cases rg -l "TestSchedulerArgParse|test.*parse.*args" -g "test_*.cpp"Length of output: 214
Script:
#!/bin/bash # Find test files fd -e cpp -e hpp test # Search for help flag handling rg -A 5 "\"help\".*program_options" # Search for port validation or error handling rg -A 10 "port.*program_options::value"Length of output: 549
Script:
#!/bin/bash # Check scheduler test files content cat tests/scheduler/test-SchedulerServer.cpp cat tests/scheduler/test-SchedulerPolicy.cpp # Look for main function implementation rg -l "int\s+main\s*\(" --type cppLength of output: 11927
Script:
#!/bin/bash # Find and check the main scheduler program file fd -e cpp scheduler$ src/ cat src/spider/scheduler/scheduler.cppLength of output: 8522
src/spider/worker/worker.cpp (3)
8-8: LGTM!The addition of the
<iostream>header is necessary for the new error output functionality.
360-360: LGTM!The main function correctly uses the new
parse_argssignature.
56-100: Verify handling of empty --libs argument.The argument parsing logic looks good, but let's verify the handling of edge cases:
- When only
--helpis provided- When
--libsis provided without any values🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/spider/worker/worker.cpp (4)
54-55: Enhance the usage message with more descriptive information.The current usage message is concise but could be more helpful. Consider adding brief descriptions of what each argument expects.
-char const* const cUsage - = "Usage: spider_worker --host <host> --storage_url <storage_url> --libs <libs>"; +char const* const cUsage + = "Usage: spider_worker --host <host_address> --storage_url <mysql_url> --libs <path_to_libraries>\n" + "Example: spider_worker --host localhost:8080 --storage_url mysql://user:pass@localhost/spider --libs /path/to/lib1.so,/path/to/lib2.so";
103-108: Standardize error message format.Error messages should follow a consistent format and provide clear guidance.
- std::cerr << "spider_worker: " << e.what() << "\n"; - std::cerr << cUsage << "\n"; - std::cerr << "Try 'spider_worker --help' for more information.\n"; + std::cerr << "Error: " << e.what() << "\n" + << cUsage << "\n" + << "For more information, run: spider_worker --help\n";
368-368: Maintain consistency in variable naming.The variable
worker_addris used for thehostparameter, which could be confusing. Consider using consistent naming throughout the codebase.- if (!parse_args(argc, argv, worker_addr, storage_url, libs)) { + if (!parse_args(argc, argv, host_addr, storage_url, libs)) {Also update the variable declaration above:
- std::string worker_addr; + std::string host_addr;
57-108: Consider implementing structured error handling.The current error handling approach mixes concerns between argument parsing, validation, and error reporting. Consider implementing a dedicated error handling structure.
Suggestions:
- Create a dedicated
ArgumentParserclass to encapsulate parsing logic- Define an
ArgumentErrorclass hierarchy for different types of errors- Implement a separate error formatter for consistent error message formatting
This would improve maintainability and make it easier to add new argument validation rules in the future.
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/spider/worker/worker.cpp(3 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spider/worker/worker.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: lint
| if (!variables.contains("host") && !variables.contains("storage_url") | ||
| && !variables.contains("libs")) | ||
| { | ||
| std::cout << cUsage << "\n"; | ||
| std::cout << desc << "\n"; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix the argument presence check logic.
The current condition is incorrect as it checks if all arguments are missing. It should check if any required argument is missing.
- if (!variables.contains("host") && !variables.contains("storage_url")
- && !variables.contains("libs"))
+ if (variables.count("help")) {
+ std::cout << desc << "\n";
+ return false;
+ }
+
+ if (!variables.contains("host") || !variables.contains("storage_url")
+ || !variables.contains("libs"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!variables.contains("host") && !variables.contains("storage_url") | |
| && !variables.contains("libs")) | |
| { | |
| std::cout << cUsage << "\n"; | |
| std::cout << desc << "\n"; | |
| return false; | |
| } | |
| if (variables.count("help")) { | |
| std::cout << desc << "\n"; | |
| return false; | |
| } | |
| if (!variables.contains("host") || !variables.contains("storage_url") | |
| || !variables.contains("libs")) | |
| { | |
| std::cout << cUsage << "\n"; | |
| std::cout << desc << "\n"; | |
| return false; | |
| } |
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
Description
spider_workerandspider_schedulerdo not print help message when--helpis required or no argument is provided. They also throw uncaught messages when value is missing or unknown argument is provided. Improve the command line argument handling to resolve #51.Validation performed
spider_workerandspider_schedulerprint help message when--helpis providedspider_workerandspider_schedulerprint help message when no argument is providedspider_workerandspider_schedulerprint meaningful error message when argument or value missingspider_workerandspider_schedulerprint meaningful error message when unknown argument is providedSummary by CodeRabbit
New Features
Bug Fixes
Refactor