Fix: Handle CLI argument parsing for --plan parameter #115
Conversation
The --plan parameter was being ignored due to disabled CLI parsing in settings_customise_sources. This fix manually parses CLI arguments in load_with_last_used method to ensure command-line parameters take effect properly. - Parse CLI arguments manually in load_with_last_used - Handle both value parameters and boolean flags correctly - Add test case to verify plan parameter parsing - Maintain backward compatibility with existing functionality Fixes the issue where running `python3 -m claude_monitor --plan max20` would still show "custom" plan instead of "max20". 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
@Maciek-roboblog This fixes a critical bug where CLI arguments are completely ignored. Would appreciate a priority review as this blocks users from selecting their subscription plans via command line. |
![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 (3)
src/claude_monitor/core/settings.py (2)
272-296: Consider enhancing the CLI argument parser to handle additional scenarios.The manual CLI parsing implementation works for the basic use case but has several limitations:
- Doesn't support equals-sign syntax (e.g.,
--plan=max20)- No error handling for unrecognized arguments
- Doesn't support short flags
- The boolean flag detection could misinterpret valid values that start with
--Consider using a more robust parsing approach:
# Parse CLI arguments manually cli_args = {} cli_provided_fields = set() if argv: i = 0 while i < len(argv): arg = argv[i] if arg.startswith("--"): - field_name = arg[2:].replace("-", "_") + # Handle both --key=value and --key value formats + if "=" in arg: + key, value = arg.split("=", 1) + field_name = key[2:].replace("-", "_") + if field_name in cls.model_fields: + cli_provided_fields.add(field_name) + cli_args[field_name] = value + i += 1 + else: + field_name = arg[2:].replace("-", "_") if field_name in cls.model_fields: cli_provided_fields.add(field_name) # Check if this is a boolean flag or requires a value if i + 1 < len(argv) and not argv[i + 1].startswith("--"): # Has a value cli_args[field_name] = argv[i + 1] i += 2 else: # Boolean flag cli_args[field_name] = True i += 1 else: + # Log or handle unrecognized arguments + logger.debug(f"Unrecognized argument: {arg}") i += 1 else: i += 1
308-310: Document why "plan" is excluded from last_params.The code excludes "plan" from being loaded from last_params, but the reason isn't documented. This design decision should be explained for future maintainers.
Add a comment explaining the rationale:
for key, value in last_params.items(): + # Skip "plan" to ensure it's always explicitly set via CLI or defaults to "custom" if key == "plan": continuesrc/tests/test_settings.py (1)
576-594: Expand test coverage for the CLI plan parsing.The test validates the basic functionality but could be more comprehensive to ensure robustness.
Consider adding these test cases:
def test_load_with_last_used_plan_parsing_comprehensive( self, mock_timezone: Mock, mock_time_format: Mock ) -> None: """Test comprehensive plan parameter parsing scenarios.""" mock_timezone.return_value = "UTC" mock_time_format.return_value = "24h" with patch("claude_monitor.core.settings.LastUsedParams") as MockLastUsed: mock_instance = Mock() MockLastUsed.return_value = mock_instance # Test all valid plan values including "custom" for plan in ["pro", "max5", "max20", "custom"]: mock_instance.load.return_value = {} settings = Settings.load_with_last_used(["--plan", plan]) assert settings.plan == plan # Test CLI overrides last_used plan mock_instance.load.return_value = {"plan": "pro"} # This should be ignored settings = Settings.load_with_last_used(["--plan", "max20"]) assert settings.plan == "max20" # Test without CLI plan argument (should default to "custom") mock_instance.load.return_value = {} settings = Settings.load_with_last_used([]) assert settings.plan == "custom"
The --plan parameter was being ignored due to disabled CLI parsing in settings_customise_sources. This fix manually parses CLI arguments in load_with_last_used method to ensure command-line parameters take effect properly.
Fixes the issue where running
python3 -m claude_monitor --plan max20would still show "custom" plan instead of "max20".🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests