-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/gh47 custom argument parser #51
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
|
All contributors have signed the CLA ✍️ ✅ |
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 custom argument parser (XTSArgumentParser) to replace standard argparse.ArgumentParser usage throughout the codebase. The custom parser integrates rich-argparse for enhanced help formatting and provides a custom error handler that displays help messages on argument errors.
Key changes:
- Created new
XTSArgumentParserclass that extendsargparse.ArgumentParserwith RichHelpFormatter - Updated all argument parser instantiations across xts.py, xts_alias, base_plugin.py, and xts_allocator_client.py
- Improved exception handling to use
ImportErrorinstead of bareexcept
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xts_core/xts_arg_parser.py | New custom argument parser class with rich formatting and custom error handling |
| src/xts_core/xts.py | Updated to use XTSArgumentParser and improved exception handling |
| src/xts_core/plugins/xts_allocator_client.py | Updated imports and parser instantiations to use XTSArgumentParser |
| src/xts_core/plugins/base_plugin.py | Updated to use XTSArgumentParser and export it for plugin use |
| pyproject.toml | Added rich-argparse dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """XTS Specific argument parser. | ||
| """ | ||
|
|
||
| def __init__(self,*args, **kwargs): |
Copilot
AI
Nov 18, 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.
Missing space after comma in parameter list. Should be def __init__(self, *args, **kwargs):
| def __init__(self,*args, **kwargs): | |
| def __init__(self, *args, **kwargs): |
| """ | ||
|
|
||
| def __init__(self,*args, **kwargs): | ||
| kwargs.pop('formatter_class','') |
Copilot
AI
Nov 18, 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.
The kwargs.pop('formatter_class', '') on line 34 removes the formatter_class if present, but uses an empty string as default. This should use None as the default value instead, as empty string is not a meaningful default for a class parameter: kwargs.pop('formatter_class', None)
| kwargs.pop('formatter_class','') | |
| kwargs.pop('formatter_class', None) |
| from rich.table import Table | ||
| try: | ||
| from base_plugin import BaseXTSPlugin, plugin_utils | ||
| from base_plugin import BaseXTSPlugin, plugin_utils, XTSArgumentParser |
Copilot
AI
Nov 18, 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.
Missing argparse import. The file uses argparse.ArgumentParser on line 176 (add_parser = allocator_subparsers.add_parser(...)) but no longer imports argparse after removing it. While XTSArgumentParser replaces most uses, the add_parser method call still requires argparse types or the XTSArgumentParser should be used there as well.
| try: | ||
| from xts_core.xts_arg_parser import XTSArgumentParser | ||
| except ImportError: | ||
| from xts_arg_parser import XTSArgumentParser |
Copilot
AI
Nov 18, 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.
Missing argparse import. The base_plugin.py file still needs to import argparse since it's likely used elsewhere in the file (e.g., for type hints, argument groups, or other argparse functionality). Only the ArgumentParser class is being replaced.
| formatter_class=RichHelpFormatter) | ||
|
|
||
| def error(self, message): | ||
| sys.stderr.write('error: %s\n' % message) |
Copilot
AI
Nov 18, 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] Using old-style string formatting (%). Consider using f-strings for consistency with modern Python: sys.stderr.write(f'error: {message}\\n')
| sys.stderr.write('error: %s\n' % message) | |
| sys.stderr.write(f'error: {message}\n') |
5909d8c to
f513782
Compare
closes #47
Created custom argument parser and changed all argument parser used in xts, alias and allocator client to use the custom parser.