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
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
src/spider/worker/worker.cpp
Outdated
| 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)
src/spider/scheduler/scheduler.cpp
Outdated
| // clang-format off | ||
| desc.add_options() | ||
| ("help", "spider scheduler") | ||
| ( | ||
| "host", |
There was a problem hiding this comment.
Let's replace these argument names with constants since we're using them in multiple places.
src/spider/scheduler/scheduler.cpp
Outdated
| if (!variables.contains("host") && !variables.contains("port") | ||
| && !variables.contains("storage_url")) |
There was a problem hiding this comment.
- Boost's program-options already validates the presence of these arguments since we marked them required.
- We should validate that the user doesn't pass in an empty value for
hostandstorage-url. If they do, we should print a specific error message for each one.
There was a problem hiding this comment.
- Boost's program-options validation is not flexible enough. For example, if user does not any argument or only provide
--help, Boost cannot correctly print the help message but treat it as arguments not provided and throw exceptions. - I'll add more validation for arguments.
There was a problem hiding this comment.
Similar comments as on the other file.
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/src/dev-docs/testing.md(1 hunks)docs/src/user-docs/guides-quick-start.md(4 hunks)src/spider/CMakeLists.txt(3 hunks)src/spider/scheduler/scheduler.cpp(4 hunks)src/spider/utils/ProgramOptions.hpp(1 hunks)src/spider/worker/TaskExecutor.hpp(3 hunks)src/spider/worker/task_executor.cpp(3 hunks)src/spider/worker/worker.cpp(4 hunks)tests/client/client-test.cpp(2 hunks)tests/integration/test_client.py(2 hunks)tests/integration/test_scheduler_worker.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/integration/test_scheduler_worker.py
- tests/integration/test_client.py
- docs/src/dev-docs/testing.md
- docs/src/user-docs/guides-quick-start.md
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/spider/worker/task_executor.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/scheduler/scheduler.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/worker/worker.cpp
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 116-116: Variable 'm_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 (7)
tests/client/client-test.cpp (1)
23-27: LGTM!The changes correctly replace string literals with constants from ProgramOptions.hpp, improving maintainability and consistency.
Also applies to: 57-61
src/spider/worker/TaskExecutor.hpp (1)
56-63: LGTM!The changes correctly use fmt::format for constructing command line arguments and consistently use the new constants from ProgramOptions.hpp.
Also applies to: 104-111
src/spider/worker/task_executor.cpp (1)
38-58: LGTM!The changes correctly replace string literals with constants from ProgramOptions.hpp, improving maintainability and consistency. The error handling remains robust.
Also applies to: 94-110
src/spider/scheduler/scheduler.cpp (1)
43-107: LGTM! The command-line argument handling has been improved.The changes align with the PR objectives by:
- Using constants from
ProgramOptions.hppfor consistent naming.- Displaying help message when no arguments are provided.
- Providing meaningful error messages for missing or invalid arguments.
- Validating required arguments and empty values.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
src/spider/worker/worker.cpp (2)
56-125: LGTM! The command-line argument handling has been improved.The changes align with the PR objectives by:
- Using constants from
ProgramOptions.hppfor consistent naming.- Displaying help message when no arguments are provided.
- Providing meaningful error messages for missing or invalid arguments.
- Validating required arguments and empty values.
🧰 Tools
🪛 Cppcheck (2.10-2)
[performance] 97-97: Variable 'id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
[performance] 116-116: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
92-95:⚠️ Potential issueFix 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 or if help is requested.
Apply this diff to fix the logic:
- if (!variables.contains(spider::core::cHostOption.data()) - && !variables.contains(spider::core::cStorageUrlOption.data()) - && !variables.contains(spider::core::cLibsOption.data())) + if (variables.count(spider::core::cHelpOption.data())) { + std::cout << spider::core::cWorkerUsage << "\n"; + std::cout << desc << "\n"; + return false; + } + + if (!variables.contains(spider::core::cHostOption.data()) + || !variables.contains(spider::core::cStorageUrlOption.data()) + || !variables.contains(spider::core::cLibsOption.data()))Likely invalid or redundant comment.
src/spider/CMakeLists.txt (1)
57-57: LGTM! The CMake changes support the improved command-line argument handling.The changes correctly add
ProgramOptions.hppto the required source sets, ensuring that the command-line argument handling improvements are properly integrated into the build system.Also applies to: 69-69, 116-118
| constexpr std::string_view cWorkerUsage | ||
| = {"Usage: spider_worker --host <host> --storage-url <storage_url> --libs <libs>"}; |
There was a problem hiding this comment.
Fix inconsistent argument naming in worker usage message.
The usage message shows storage_url with an underscore, but the actual option is defined with a hyphen as storage-url. This inconsistency could confuse users.
Apply this diff to fix the inconsistency:
- = {"Usage: spider_worker --host <host> --storage-url <storage_url> --libs <libs>"};
+ = {"Usage: spider_worker --host <host> --storage-url <storage-url> --libs <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.
| constexpr std::string_view cWorkerUsage | |
| = {"Usage: spider_worker --host <host> --storage-url <storage_url> --libs <libs>"}; | |
| constexpr std::string_view cWorkerUsage | |
| = {"Usage: spider_worker --host <host> --storage-url <storage-url> --libs <libs>"}; |
| constexpr std::string_view cTaskExecutorUsage | ||
| = {"Usage: spider_task_executor --func <function> --task-id <task_id> --storage-url " | ||
| "<storage_url> --libs <libs>"}; |
There was a problem hiding this comment.
Fix inconsistent argument naming in task executor usage message.
The option task-id is defined with a hyphen, but task_id appears with an underscore in the usage message.
Apply this diff to fix the inconsistency:
- = {"Usage: spider_task_executor --func <function> --task-id <task_id> --storage-url "
+ = {"Usage: spider_task_executor --func <function> --task-id <task-id> --storage-url "📝 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.
| constexpr std::string_view cTaskExecutorUsage | |
| = {"Usage: spider_task_executor --func <function> --task-id <task_id> --storage-url " | |
| "<storage_url> --libs <libs>"}; | |
| constexpr std::string_view cTaskExecutorUsage | |
| = {"Usage: spider_task_executor --func <function> --task-id <task-id> --storage-url " | |
| "<storage_url> --libs <libs>"}; |
src/spider/scheduler/scheduler.cpp
Outdated
| if (false == variables.contains(spider::core::cHostOption.data()) | ||
| && false == variables.contains(spider::core::cPortOption.data()) | ||
| && false == variables.contains(spider::core::cStorageUrlOption.data())) | ||
| { |
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 or if help is requested.
Apply this diff to fix the logic:
- if (false == variables.contains(spider::core::cHostOption.data())
- && false == variables.contains(spider::core::cPortOption.data())
- && false == variables.contains(spider::core::cStorageUrlOption.data()))
+ if (variables.count(spider::core::cHelpOption.data())) {
+ std::cout << spider::core::cSchedulerUsage << "\n";
+ std::cout << desc << "\n";
+ return false;
+ }
+
+ if (false == variables.contains(spider::core::cHostOption.data())
+ || false == variables.contains(spider::core::cPortOption.data())
+ || false == variables.contains(spider::core::cStorageUrlOption.data()))📝 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 (false == variables.contains(spider::core::cHostOption.data()) | |
| && false == variables.contains(spider::core::cPortOption.data()) | |
| && false == variables.contains(spider::core::cStorageUrlOption.data())) | |
| { | |
| if (variables.count(spider::core::cHelpOption.data())) { | |
| std::cout << spider::core::cSchedulerUsage << "\n"; | |
| std::cout << desc << "\n"; | |
| return false; | |
| } | |
| if (false == variables.contains(spider::core::cHostOption.data()) | |
| || false == variables.contains(spider::core::cPortOption.data()) | |
| || false == variables.contains(spider::core::cStorageUrlOption.data())) | |
| { |
sitaowang1998
changed the title
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
--storage_urlto--storage-url.Refactor