Enhance CLI with global settings management and subcommand structure#19
Enhance CLI with global settings management and subcommand structure#19Archov wants to merge 14 commits into
Conversation
- Introduced global settings management with JSON configuration for user preferences, including verbosity and thread count. - Refactored CLI to implement a subcommand structure for better organization, allowing commands like `convert`, `watch`, `debug`, `scan`, `config`, and `stats`. - Updated argument parsing to support new subcommands and improved help formatting for clarity. - Enhanced dependency management by parsing requirements from a file, allowing for dynamic handling of required and optional dependencies. - Improved error handling and logging throughout the CLI for better user feedback and debugging capabilities.
Reviewer's GuideThis PR transforms the monolithic CLI into a modular subcommand-based tool, introduces persistent global settings via JSON, implements dynamic dependency parsing from a requirements file, and enhances logging and error handling across the codebase. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the monolithic CLI with a subcommand-driven argparse interface (convert, watch, debug, scan, config, stats); adds global settings persistence and apply/load functions, requirements-driven dependency checks and installers, modular command handlers with public entrypoints, updated requirements, editor settings, .gitignore tweaks, and governance docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as cbxtools.main()
participant A as argparse (subparsers)
participant G as GlobalSettings
participant D as DependencyChecker
participant S as StatsTracker
participant H as CommandHandler
rect rgb(240,248,255)
U->>C: cbxtools <subcommand> [args]
C->>A: parse_arguments() → bound handler
C->>G: load_global_settings() / apply_global_settings()
end
alt subcommand ∉ {config, stats}
rect rgb(255,250,240)
C->>D: check_and_install_dependencies(auto_install?)
D-->>C: missing/installed status
end
end
opt stats enabled
C->>S: init tracker
end
C->>H: dispatch handler(args, logger, stats?)
H-->>C: result / exit code
C-->>U: program exit
sequenceDiagram
autonumber
participant D as check_and_install_dependencies
participant R as parse_requirements_file
participant N as normalize_package_name
participant P as offer_to_install_dependencies
participant I as install_dependencies
rect rgb(245,255,250)
D->>R: read requirements.txt
R-->>D: parsed dependency list
D->>N: normalize package names
D->>D: compute missing deps
end
alt missing deps
D->>P: prompt user / auto-install
alt confirmed / auto
P->>I: run installer subprocesses
I-->>P: install results
else declined
P-->>D: skip installation
end
else none missing
D-->>D: nothing to install
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Summary of ChangesHello @Archov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant architectural overhaul of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `cbxtools/cli.py:440-449` </location>
<code_context>
+def install_dependencies(deps_to_install, logger):
</code_context>
<issue_to_address>
**suggestion:** The installation logic uses 'requirement' key if present, but this key is not set in parse_requirements_file.
Standardize the dependency dictionary to consistently use 'package_name', or ensure 'requirement' is always set to avoid future inconsistencies.
Suggested implementation:
```python
def install_dependencies(deps_to_install, logger):
"""
Install dependencies using pip.
Args:
deps_to_install: List of dependency info dicts to install
```
```python
Args:
deps_to_install: List of dependency info dicts to install
"""
for dep in deps_to_install:
package_name = dep.get('package_name')
if not package_name:
logger.error(f"Dependency dictionary missing 'package_name': {dep}")
continue
# Install using pip
try:
subprocess.check_call([sys.executable, "-m", "pip", "install", package_name])
logger.info(f"Successfully installed {package_name}")
except Exception as e:
logger.error(f"Failed to install {package_name}: {e}")
```
You will also need to:
1. Update the `parse_requirements_file` function (or wherever dependencies are parsed) to ensure each dependency dictionary includes a `'package_name'` key, and remove any usage of `'requirement'`.
2. If other parts of the codebase reference `'requirement'`, refactor them to use `'package_name'` for consistency.
</issue_to_address>
### Comment 2
<location> `cbxtools/cli.py:569-93` </location>
<code_context>
+def parse_arguments():
</code_context>
<issue_to_address>
**issue (bug_risk):** Subcommand argument parsing may result in ambiguous or missing required arguments.
Currently, required arguments like input_path and output_dir are not explicitly validated after parsing, which may cause runtime errors if omitted. Please add post-parsing checks to ensure all required arguments are provided for each subcommand.
</issue_to_address>
### Comment 3
<location> `cbxtools/cli.py:725-734` </location>
<code_context>
+def _add_convert_arguments(parser):
</code_context>
<issue_to_address>
**nitpick:** Multiple argument groups with the same name may cause confusion in help output.
There are two 'Processing' argument groups in _add_convert_arguments; renaming one will help distinguish their roles in the help output.
</issue_to_address>
### Comment 4
<location> `cbxtools/cli.py:1248-1257` </location>
<code_context>
+ # Enable stats by default for other commands
+ stats_tracker = StatsTracker()
-def main():
- args = parse_arguments()
- logger = setup_logging(args.verbose, args.silent)
-
- # Handle dependency operations first
- dependency_result = handle_dependency_operations(args, logger)
- if dependency_result is not None:
- return dependency_result
- # Check dependencies early unless user explicitly wants to skip
- if not getattr(args, 'skip_dependency_check', False):
- dep_status = check_and_install_dependencies(logger, auto_install=False)
- if not dep_status['all_required_available']:
- if not dep_status.get('user_declined', False):
- logger.error("Required dependencies are missing. Please install them and try again.")
</code_context>
<issue_to_address>
**suggestion:** Dependency checking is performed unconditionally, which may block config or help commands.
Consider updating the logic to bypass dependency checks for commands like config and help, allowing users to access these features even if dependencies are missing.
</issue_to_address>
### Comment 5
<location> `cbxtools/cli.py:1076-459` </location>
<code_context>
+def handle_config_presets(args, logger):
</code_context>
<issue_to_address>
**issue (bug_risk):** Importing presets does not handle exceptions from import_presets_from_file.
Wrap import_presets_from_file in a try/except block and log errors to prevent crashes from malformed files.
</issue_to_address>
### Comment 6
<location> `cbxtools/cli.py:468-473` </location>
<code_context>
subprocess.run(
[sys.executable, '-m', 'pip', '--version'],
capture_output=True,
check=True,
timeout=30,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `cbxtools/cli.py:83-85` </location>
<code_context>
if not hasattr(args, 'verbose') or not args.verbose:
if settings.get('verbose', False):
args.verbose = True
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if (not hasattr(args, 'verbose') or not args.verbose) and settings.get('verbose', False):
args.verbose = True
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 8
<location> `cbxtools/cli.py:86-88` </location>
<code_context>
if not hasattr(args, 'silent') or not args.silent:
if settings.get('silent', False):
args.silent = True
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if (not hasattr(args, 'silent') or not args.silent) and settings.get('silent', False):
args.silent = True
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 9
<location> `cbxtools/cli.py:89-91` </location>
<code_context>
if not hasattr(args, 'threads') or args.threads is None:
if 'threads' in settings:
args.threads = settings['threads']
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if (not hasattr(args, 'threads') or args.threads is None) and 'threads' in settings:
args.threads = settings['threads']
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 10
<location> `cbxtools/cli.py:172` </location>
<code_context>
'description': comment_part if comment_part else f'{import_lookup_key} package',
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if-expression with `or` ([`or-if-exp-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/or-if-exp-identity))
```suggestion
'description': comment_part or f'{import_lookup_key} package',
```
<br/><details><summary>Explanation</summary>Here we find ourselves setting a value if it evaluates to `True`, and otherwise
using a default.
The 'After' case is a bit easier to read and avoids the duplication of
`input_currency`.
It works because the left-hand side is evaluated first. If it evaluates to
true then `currency` will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
`currency` will be set to `DEFAULT_CURRENCY`.
</details>
</issue_to_address>
### Comment 11
<location> `cbxtools/cli.py:741` </location>
<code_context>
quality_group.add_argument('--method', type=int, choices=range(0, 7), default=4,
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace range(0, x) with range(x) ([`remove-zero-from-range`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/remove-zero-from-range))
```suggestion
quality_group.add_argument('--method', type=int, choices=range(7), default=4,
```
<br/><details><summary>Explanation</summary>The default starting value for a call to `range()` is 0, so it is unnecessary to
explicitly define it. This refactoring removes such zeros, slightly shortening
the code.
</details>
</issue_to_address>
### Comment 12
<location> `cbxtools/cli.py:799` </location>
<code_context>
output_group.add_argument('--compression', type=int, choices=range(0, 10), default=6,
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace range(0, x) with range(x) ([`remove-zero-from-range`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/remove-zero-from-range))
```suggestion
output_group.add_argument('--compression', type=int, choices=range(10), default=6,
```
<br/><details><summary>Explanation</summary>The default starting value for a call to `range()` is 0, so it is unnecessary to
explicitly define it. This refactoring removes such zeros, slightly shortening
the code.
</details>
</issue_to_address>
### Comment 13
<location> `cbxtools/cli.py:1279-1282` </location>
<code_context>
if not dep_status['all_required_available']:
if not dep_status.get('user_declined', False):
logger.error("Required dependencies are missing. Please install them and try again.")
return 1
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if not dep_status['all_required_available'] and not dep_status.get('user_declined', False):
logger.error("Required dependencies are missing. Please install them and try again.")
return 1
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 14
<location> `cbxtools/cli.py:410-420` </location>
<code_context>
def offer_to_install_dependencies(missing_deps, logger):
"""
Offer to install missing dependencies interactively.
Args:
missing_deps: List of missing dependency info dicts
logger: Logger instance
Returns:
dict: Installation results
"""
logger.info("\nMissing dependencies detected:")
for dep in missing_deps:
logger.info(f" - {dep['package_name']}: {dep['description']}")
logger.info("\nOptions:")
logger.info(" 1. Install all missing dependencies automatically")
logger.info(" 2. Install only required dependencies")
logger.info(" 3. Install manually with: pip install " + " ".join(dep['package_name'] for dep in missing_deps))
logger.info(" 4. Continue without installing (some features may not work)")
try:
choice = input("\nChoose an option (1-4): ").strip()
if choice == '1':
return install_dependencies(missing_deps, logger)
elif choice == '2':
# Filter for required dependencies only
required_packages = ['pillow', 'rarfile', 'py7zr']
# Normalize both dep package names and required packages for comparison
required_package_names_lower = [pkg.lower() for pkg in required_packages]
required_deps = [dep for dep in missing_deps if normalize_package_name(dep['package_name']) in required_package_names_lower]
if not required_deps:
logger.info("No required dependencies found in missing dependencies list.")
logger.info("Continuing without installing dependencies...")
return {'all_required_available': False, 'user_declined': True}
return install_dependencies(required_deps, logger)
elif choice == '3':
packages = " ".join(dep['package_name'] for dep in missing_deps)
logger.info(f"\nTo install manually, run this command:")
logger.info(f" pip install {packages}")
logger.info("\nOr use the built-in installer:")
logger.info(f" {sys.argv[0]} --install-dependencies")
return {'all_required_available': False, 'user_declined': True}
else:
logger.info("Continuing without installing dependencies...")
return {'all_required_available': False, 'user_declined': True}
except (KeyboardInterrupt, EOFError):
logger.info("\nInstallation cancelled by user.")
return {'all_required_available': False, 'user_declined': True}
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into function ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 15
<location> `cbxtools/cli.py:547` </location>
<code_context>
def _format_usage(self, usage, actions, groups, prefix):
"""Override to provide better usage line formatting."""
if prefix is None:
prefix = 'usage: '
# Get the program name
if self._prog:
prog = self._prog
else:
prog = '%(prog)s'
# Create a more readable usage line for the new subcommand structure
usage_text = f"{prefix}{prog} [GLOBAL_OPTIONS] <command> [COMMAND_OPTIONS]"
return usage_text
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
- Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))
</issue_to_address>
### Comment 16
<location> `cbxtools/cli.py:1146` </location>
<code_context>
def handle_config_settings(args, logger):
"""Handle config settings subcommand."""
if args.init:
logger.error("Settings file creation not yet implemented")
return 1
elif args.save:
logger.error("Settings file saving not yet implemented")
return 1
else:
logger.error("No settings operation specified")
return 1
</code_context>
<issue_to_address>
**issue (code-quality):** Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| def handle_convert_command(args, logger, stats_tracker=None): | ||
|
|
||
| """Handle the convert command.""" | ||
|
|
||
| logger.error("Convert command not yet implemented") |
There was a problem hiding this comment.
[P0] Replace functional CLI commands with unimplemented stubs
The new subcommand dispatch calls handle_convert_command, but that handler now only logs "Convert command not yet implemented" and exits with an error. The same file also stubs out watch, debug, and scan in the same way, so every primary CLI operation now immediately fails instead of processing archives as it did before this commit. This is a complete regression for users invoking cbxtools convert or related commands.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cbxtools/cli.py(1 hunks)debug_parse.py(1 hunks)requirements.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cbxtools/cli.py
📄 CodeRabbit inference engine (AGENTS.md)
cbxtools/cli.py: CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Files:
cbxtools/cli.py
cbxtools/@(near_greyscale_scan|cli).py
📄 CodeRabbit inference engine (AGENTS.md)
For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Files:
cbxtools/cli.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.075Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.075Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.075Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
📚 Learning: 2025-09-26T03:27:42.075Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.075Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.075Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.075Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.075Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.075Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.075Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.075Z
Learning: Applies to cbxtools/debug_utils.py : In debug mode, only auto-greyscale thresholds from presets are used; ignore other conversion parameters
Applied to files:
cbxtools/cli.py
🧬 Code graph analysis (1)
cbxtools/cli.py (8)
cbxtools/utils.py (2)
setup_logging(12-26)log_effective_parameters(42-74)cbxtools/core/path_validator.py (1)
PathValidator(8-151)cbxtools/core/filesystem_utils.py (2)
FileSystemUtils(11-145)cleanup_empty_directories(78-114)cbxtools/core/file_processor.py (2)
FileProcessor(17-254)find_processable_items(257-291)cbxtools/conversion.py (6)
process_single_file(420-529)process_archive_files(532-652)info(246-246)debug(245-245)warning(247-247)error(248-248)cbxtools/stats_tracker.py (3)
StatsTracker(14-104)print_summary_report(107-156)print_lifetime_stats(159-173)cbxtools/watchers.py (2)
watch_directory(76-424)cleanup_empty_directories(30-32)cbxtools/presets.py (5)
list_available_presets(89-92)apply_preset_with_overrides(216-255)export_preset_from_args(257-282)save_preset(94-137)import_presets_from_file(139-187)
🪛 Ruff (0.13.1)
cbxtools/cli.py
74-74: Consider moving this statement to an else block
(TRY300)
221-221: Loop control variable category not used within loop body
Rename unused category to _category
(B007)
223-223: Loop control variable name not used within loop body
(B007)
412-412: f-string without any placeholders
Remove extraneous f prefix
(F541)
468-468: subprocess call: check for execution of untrusted input
(S603)
475-475: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
476-476: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
477-477: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
482-482: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
484-484: subprocess call: check for execution of untrusted input
(S603)
497-497: f-string without any placeholders
Remove extraneous f prefix
(F541)
515-515: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
519-519: Do not catch blind exception: Exception
(BLE001)
521-521: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
535-535: Unused method argument: usage
(ARG002)
535-535: Unused method argument: actions
(ARG002)
535-535: Unused method argument: groups
(ARG002)
1001-1001: Local variable show_parser is assigned to but never used
Remove assignment to unused variable show_parser
(F841)
1004-1004: Local variable reset_parser is assigned to but never used
Remove assignment to unused variable reset_parser
(F841)
1007-1007: Local variable disable_parser is assigned to but never used
Remove assignment to unused variable disable_parser
(F841)
1016-1016: Unused function argument: args
(ARG001)
1016-1016: Unused function argument: stats_tracker
(ARG001)
1028-1028: Unused function argument: args
(ARG001)
1028-1028: Unused function argument: stats_tracker
(ARG001)
1040-1040: Unused function argument: args
(ARG001)
1052-1052: Unused function argument: args
(ARG001)
1064-1064: Unused function argument: args
(ARG001)
1303-1303: Local variable func is assigned to but never used
Remove assignment to unused variable func
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring of the CLI. The move to a subcommand structure greatly improves the usability and organization of the tool, and the dynamic dependency management based on requirements.txt is a robust enhancement. I've identified a few areas for improvement, mainly around error handling, maintainability, and some dead code. The addition of global settings is also a great feature, though its command-line interface could be more explicit. Overall, this is a fantastic step forward for the project.
| except (json.JSONDecodeError, IOError): | ||
| pass |
There was a problem hiding this comment.
Silently passing on json.JSONDecodeError or IOError can hide issues with a corrupted or unreadable settings file. This could lead to unexpected behavior if the user thinks their settings are being applied when they are not. It would be better to log a warning to inform the user about the problem.
| except (json.JSONDecodeError, IOError): | |
| pass | |
| except (json.JSONDecodeError, IOError) as e: | |
| logging.warning(f"Could not load global settings from {DEFAULT_SETTINGS_FILE}: {e}") | |
| pass |
| except IOError: | ||
| return False |
There was a problem hiding this comment.
Silently returning False on IOError provides no feedback to the user about why saving the global settings failed. It would be beneficial to log the error to help with debugging.
| except IOError: | |
| return False | |
| except IOError as e: | |
| logging.error(f"Failed to save global settings: {e}") | |
| return False |
| # Compute import-lookup key by stripping extras and version operators | ||
| # Remove extras in [...] and split on version operators to get base package name | ||
| import_lookup_key = requirement_part.split('[')[0] # Remove extras | ||
| import_lookup_key = import_lookup_key.split('>=')[0].split('==')[0].split('<=')[0].split('<')[0].split('>')[0].split('!=')[0].split('~=')[0].strip() |
There was a problem hiding this comment.
This long chain of .split() calls is hard to read and maintain. It could be simplified by using re.split to split on any of the version operators. This would make the code more robust and easier to understand.
| import_lookup_key = import_lookup_key.split('>=')[0].split('==')[0].split('<=')[0].split('<')[0].split('>')[0].split('!=')[0].split('~=')[0].strip() | |
| import_lookup_key = re.split(r'[>=<~!]=?', import_lookup_key)[0].strip() |
| for separator in ['>=', '==', '<=', '!=', '~=', '<', '>']: | ||
| normalized = normalized.split(separator)[0] |
There was a problem hiding this comment.
This loop for splitting on version separators can be made more concise and efficient by using a regular expression. This improves readability and maintainability.
| for separator in ['>=', '==', '<=', '!=', '~=', '<', '>']: | |
| normalized = normalized.split(separator)[0] | |
| import re | |
| normalized = re.split(r'[>=<~!]=?', package_name)[0] |
|
|
||
| # Filter for required dependencies only | ||
|
|
||
| required_packages = ['pillow', 'rarfile', 'py7zr'] |
There was a problem hiding this comment.
The required_packages list is hardcoded. This is fragile and duplicates information that is now available in requirements.txt. To improve maintainability, this list should be derived dynamically from the parsed dependencies, for instance by inspecting the 'required' category from the dependencies dictionary.
| """ | ||
| packages = [dep['package_name'] for dep in deps_to_install] | ||
|
|
||
| packages = [dep.get('requirement', dep['package_name']) for dep in deps_to_install] |
There was a problem hiding this comment.
This line uses dep.get('requirement', dep['package_name']), but the key 'requirement' is never added to the dependency dictionary in parse_requirements_file. This appears to be unused code. If it's intended for future use, it should be documented; otherwise, it should be simplified to just use dep['package_name'] to avoid confusion.
| packages = [dep.get('requirement', dep['package_name']) for dep in deps_to_install] | |
| packages = [dep['package_name'] for dep in deps_to_install] |
| except Exception as e: | ||
|
|
||
| logger.error(f"Error during installation: {e}") | ||
|
|
||
| return {'all_required_available': False, 'installation_error': str(e)} |
There was a problem hiding this comment.
Catching a generic Exception is too broad and can mask unexpected errors. It's better to catch more specific exceptions that might occur during the subprocess call, such as OSError for issues with executing the command.
| else: | ||
| # Save global configuration options | ||
| settings = {} | ||
| if hasattr(args, 'verbose') and args.verbose: | ||
| settings['verbose'] = True | ||
| if hasattr(args, 'silent') and args.silent: | ||
| settings['silent'] = True | ||
| if hasattr(args, 'threads') and args.threads is not None: | ||
| settings['threads'] = args.threads | ||
|
|
||
| if settings: | ||
| if save_global_settings(settings): | ||
| logger.info("Global settings saved successfully") | ||
| return 0 | ||
| else: | ||
| list_file = candidate | ||
| logger.error("Failed to save global settings") | ||
| return 1 | ||
| else: | ||
| list_file = Path('near_greyscale_list.txt') | ||
|
|
||
| results, output_path = scan_directory_for_near_greyscale( | ||
| input_path, | ||
| args.recursive, | ||
| pixel_threshold, | ||
| percent_threshold, | ||
| threads, | ||
| logger, | ||
| list_file, | ||
| ) | ||
| logger.info("No settings to save. Use --verbose, --silent, or --threads to set global options.") | ||
| return 0 |
There was a problem hiding this comment.
The config command without a subcommand implicitly saves global settings (e.g., from --verbose). This behavior might be surprising to users who expect an explicit action to save settings. Consider adding an explicit subcommand like set or save-globals to make this action clearer and more intuitive.
|
|
||
| command = args.command | ||
|
|
||
| func = getattr(args, 'func', None) |
There was a problem hiding this comment.
The variable func is assigned but never used. This is dead code and should be removed to improve code clarity.
| @@ -0,0 +1 @@ | |||
|
No newline at end of file |
|||
There was a problem hiding this comment.
This new file is empty except for a single space. If it's a placeholder for future development, it would be helpful to add a comment explaining its purpose. If it's not needed, it should be removed to keep the project clean.
- Updated `.gitignore` to include `pyproject.toml` and `requirements-dev.txt` for better project management. - Introduced `.vscode/settings.json` for consistent development environment settings, including formatting and linting configurations. - Refactored `cli.py` to improve global settings management, error handling, and command structure, enhancing user experience and maintainability. - Improved argument parsing and logging for better feedback during command execution.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks).vscode/settings.json(1 hunks)cbxtools/cli.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (2)
cbxtools/cli.py
📄 CodeRabbit inference engine (AGENTS.md)
cbxtools/cli.py: CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Files:
cbxtools/cli.py
cbxtools/@(near_greyscale_scan|cli).py
📄 CodeRabbit inference engine (AGENTS.md)
For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Files:
cbxtools/cli.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Applied to files:
cbxtools/cli.py
🧬 Code graph analysis (1)
cbxtools/cli.py (5)
cbxtools/core/file_processor.py (1)
find_processable_items(257-291)cbxtools/presets.py (3)
apply_preset_with_overrides(216-255)list_available_presets(89-92)import_presets_from_file(139-187)cbxtools/stats_tracker.py (3)
StatsTracker(14-104)add_run(55-75)print_lifetime_stats(159-173)cbxtools/utils.py (1)
setup_logging(12-26)cbxtools/conversion.py (4)
error(248-248)warning(247-247)info(246-246)process_archive_files(532-652)
🪛 Ruff (0.13.1)
cbxtools/cli.py
50-50: Consider moving this statement to an else block
(TRY300)
334-334: subprocess call: check for execution of untrusted input
(S603)
345-345: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
346-346: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
347-347: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
351-351: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
353-353: subprocess call: check for execution of untrusted input
(S603)
377-377: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
379-379: Do not catch blind exception: Exception
(BLE001)
380-380: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
387-387: Unused method argument: usage
(ARG002)
387-387: Unused method argument: actions
(ARG002)
387-387: Unused method argument: groups
(ARG002)
820-820: Do not catch blind exception: Exception
(BLE001)
821-821: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
829-829: Do not catch blind exception: Exception
(BLE001)
830-830: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
844-844: Do not catch blind exception: Exception
(BLE001)
845-845: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
862-862: Do not catch blind exception: Exception
(BLE001)
863-863: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
875-875: Unpacked variable processed_files is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
899-899: Consider moving this statement to an else block
(TRY300)
900-900: Do not catch blind exception: Exception
(BLE001)
901-901: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
905-905: Unused function argument: args
(ARG001)
905-905: Unused function argument: stats_tracker
(ARG001)
911-911: Unused function argument: stats_tracker
(ARG001)
923-923: Unused function argument: stats_tracker
(ARG001)
933-933: Unused function argument: args
(ARG001)
939-939: Unused function argument: args
(ARG001)
945-945: Unused function argument: args
(ARG001)
986-986: Do not catch blind exception: Exception
(BLE001)
987-987: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
Resolves critical issues and improves code quality across CLI module: Critical Fixes: - Fix preset overwriting by parser defaults (Comment 818571431) - Fix version-pinned packages being silently dropped during install (Comment 2946190197) - Resolve P0 regression concern - CLI commands are fully implemented (Comment 557376339) Code Quality Improvements: - Replace generic Exception handling with specific exceptions (Comment 1777568110) - Remove dead code - unused func variable (Comment 2469779635) - Replace hardcoded required_packages with dynamic parsing (Comment 2022687839) - Simplify if-expressions with or operator (Comment 3209942113) - Merge nested if conditions for better readability (Comments 7-9, 13) UX Enhancements: - Improve config command messaging and documentation (Comment 2378339884) - Add explicit save confirmation for global settings Security Verification: - Verify subprocess calls are secure against command injection (Comment 2194553930)
|
@sourcery-ai which of your "code-quality" comments are causing you to mark the PR as blocked? 🤨 |
|
Great question! I don't directly block PRs—my review status is advisory, and I highlight areas that may need attention for code quality, maintainability, or correctness. If you see the PR marked as "blocked" or "changes requested," it's usually because one or more of my comments are flagged as requiring action (for example, issues with logic, error handling, or code structure). To identify which comments are blocking:
If you're unsure which comments are blocking, let me know and I can summarize the key issues that should be resolved for a clean merge. If all my comments are resolved or marked as "addressed," the PR should be good to go! Let me know if you'd like a summary of the current blocking issues, or if you have any other questions about my review process. |
- Implemented validation and sanitization of package names in the `install_dependencies` function to prevent command injection vulnerabilities. - Added logging to warn about potentially unsafe package names being skipped. - Ensured subprocess calls are secure by using a validated list format for package installation. This update improves the overall security posture of the CLI module while maintaining existing functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cursor/rules/Project.mdc(1 hunks).cursor/rules/code-supernova.mdc(1 hunks).cursor/rules/pr-comment-resolution.mdc(1 hunks)cbxtools/cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cbxtools/cli.py
📄 CodeRabbit inference engine (AGENTS.md)
cbxtools/cli.py: CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Files:
cbxtools/cli.py
cbxtools/@(near_greyscale_scan|cli).py
📄 CodeRabbit inference engine (AGENTS.md)
For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Files:
cbxtools/cli.py
🧠 Learnings (5)
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Implement preset application via apply_preset_with_overrides() to resolve CLI args over presets over defaults
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Expose new preprocessing options through the preset system so they can be configured per preset
Applied to files:
cbxtools/cli.py
🧬 Code graph analysis (1)
cbxtools/cli.py (5)
cbxtools/core/file_processor.py (1)
find_processable_items(257-291)cbxtools/presets.py (1)
apply_preset_with_overrides(216-255)cbxtools/stats_tracker.py (3)
StatsTracker(14-104)add_run(55-75)print_lifetime_stats(159-173)cbxtools/utils.py (1)
setup_logging(12-26)cbxtools/conversion.py (3)
warning(247-247)info(246-246)process_archive_files(532-652)
🪛 Ruff (0.13.1)
cbxtools/cli.py
50-50: Consider moving this statement to an else block
(TRY300)
279-279: Do not catch blind exception: Exception
(BLE001)
335-335: subprocess call: check for execution of untrusted input
(S603)
346-346: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
347-347: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
348-348: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
352-352: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
354-354: subprocess call: check for execution of untrusted input
(S603)
378-378: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
380-380: Do not catch blind exception: Exception
(BLE001)
381-381: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
388-388: Unused method argument: usage
(ARG002)
388-388: Unused method argument: actions
(ARG002)
388-388: Unused method argument: groups
(ARG002)
831-831: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
840-840: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
855-855: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
873-873: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
885-885: Unpacked variable processed_files is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
909-909: Consider moving this statement to an else block
(TRY300)
911-911: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
915-915: Unused function argument: args
(ARG001)
915-915: Unused function argument: stats_tracker
(ARG001)
921-921: Unused function argument: stats_tracker
(ARG001)
933-933: Unused function argument: stats_tracker
(ARG001)
943-943: Unused function argument: args
(ARG001)
949-949: Unused function argument: args
(ARG001)
955-955: Unused function argument: args
(ARG001)
996-996: Do not catch blind exception: Exception
(BLE001)
997-997: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
- Improved the `install_dependencies` function by adding strict validation for package names to prevent command injection vulnerabilities. - Updated the command-line interface to include new options for thread management, original file retention, and output archive creation control. - Enhanced the default settings for the convert command to improve user experience and streamline operations. These changes bolster the security and usability of the CLI module while maintaining existing functionality.
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 (1)
cbxtools/cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cbxtools/cli.py
📄 CodeRabbit inference engine (AGENTS.md)
cbxtools/cli.py: CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Files:
cbxtools/cli.py
cbxtools/@(near_greyscale_scan|cli).py
📄 CodeRabbit inference engine (AGENTS.md)
For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Files:
cbxtools/cli.py
🧠 Learnings (5)
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Implement preset application via apply_preset_with_overrides() to resolve CLI args over presets over defaults
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Expose new preprocessing options through the preset system so they can be configured per preset
Applied to files:
cbxtools/cli.py
🧬 Code graph analysis (1)
cbxtools/cli.py (5)
cbxtools/core/file_processor.py (1)
find_processable_items(257-291)cbxtools/presets.py (3)
apply_preset_with_overrides(216-255)list_available_presets(89-92)import_presets_from_file(139-187)cbxtools/stats_tracker.py (2)
StatsTracker(14-104)add_run(55-75)cbxtools/utils.py (1)
setup_logging(12-26)cbxtools/conversion.py (4)
error(248-248)warning(247-247)info(246-246)process_archive_files(532-652)
🪛 Ruff (0.13.1)
cbxtools/cli.py
50-50: Consider moving this statement to an else block
(TRY300)
279-279: Do not catch blind exception: Exception
(BLE001)
342-342: subprocess call: check for execution of untrusted input
(S603)
353-353: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
354-354: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
355-355: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
369-369: subprocess call: check for execution of untrusted input
(S603)
393-393: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
395-395: Do not catch blind exception: Exception
(BLE001)
396-396: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
403-403: Unused method argument: usage
(ARG002)
403-403: Unused method argument: actions
(ARG002)
403-403: Unused method argument: groups
(ARG002)
872-872: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
881-881: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
896-896: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
914-914: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
950-950: Consider moving this statement to an else block
(TRY300)
952-952: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
956-956: Unused function argument: args
(ARG001)
956-956: Unused function argument: stats_tracker
(ARG001)
962-962: Unused function argument: stats_tracker
(ARG001)
974-974: Unused function argument: stats_tracker
(ARG001)
984-984: Unused function argument: args
(ARG001)
990-990: Unused function argument: args
(ARG001)
996-996: Unused function argument: args
(ARG001)
1037-1037: Do not catch blind exception: Exception
(BLE001)
1038-1038: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cbxtools/cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cbxtools/cli.py
📄 CodeRabbit inference engine (AGENTS.md)
cbxtools/cli.py: CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Files:
cbxtools/cli.py
cbxtools/@(near_greyscale_scan|cli).py
📄 CodeRabbit inference engine (AGENTS.md)
For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Files:
cbxtools/cli.py
🧠 Learnings (5)
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Implement preset application via apply_preset_with_overrides() to resolve CLI args over presets over defaults
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Expose new preprocessing options through the preset system so they can be configured per preset
Applied to files:
cbxtools/cli.py
🧬 Code graph analysis (1)
cbxtools/cli.py (5)
cbxtools/core/file_processor.py (1)
find_processable_items(257-291)cbxtools/presets.py (3)
apply_preset_with_overrides(216-255)list_available_presets(89-92)import_presets_from_file(139-187)cbxtools/stats_tracker.py (3)
StatsTracker(14-104)add_run(55-75)print_lifetime_stats(159-173)cbxtools/utils.py (1)
setup_logging(12-26)cbxtools/conversion.py (4)
error(248-248)warning(247-247)info(246-246)process_archive_files(532-652)
🪛 Ruff (0.13.1)
cbxtools/cli.py
72-72: Consider moving this statement to an else block
(TRY300)
301-301: Do not catch blind exception: Exception
(BLE001)
366-366: subprocess call: check for execution of untrusted input
(S603)
377-377: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
378-378: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
379-379: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
389-389: subprocess call: check for execution of untrusted input
(S603)
413-413: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
415-415: Do not catch blind exception: Exception
(BLE001)
416-416: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
423-423: Unused method argument: usage
(ARG002)
423-423: Unused method argument: actions
(ARG002)
423-423: Unused method argument: groups
(ARG002)
892-892: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
901-901: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
916-916: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
934-934: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
970-970: Consider moving this statement to an else block
(TRY300)
972-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
976-976: Unused function argument: args
(ARG001)
976-976: Unused function argument: stats_tracker
(ARG001)
982-982: Unused function argument: stats_tracker
(ARG001)
994-994: Unused function argument: stats_tracker
(ARG001)
1004-1004: Unused function argument: args
(ARG001)
1010-1010: Unused function argument: args
(ARG001)
1016-1016: Unused function argument: args
(ARG001)
1057-1057: Do not catch blind exception: Exception
(BLE001)
1058-1058: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
cbxtools/cli.py (3)
85-90: Persisted thread count is ignored
convert_parserseedsthreads=0, so this guard never fires and a savedthreadspreference is lost. Repro:cbxtools config --threads 4thencbxtools convert …still runs with 0 threads. Treat the parser default (0) as “unspecified” unless--threadsappeared on the command line so the saved value can win.- if (not hasattr(args, "threads") or args.threads is None) and "threads" in settings: - args.threads = settings["threads"] + if ( + not hasattr(args, "threads") + or (args.threads in (None, 0) and "--threads" not in sys.argv) + ) and "threads" in settings: + args.threads = settings["threads"]
1172-1179: Global verbosity settings are never honored
setup_loggingis called beforeapply_global_settings, so persistedverbose/silentflags never influence the logging level. Example: put{"verbose": true}insettings.json, runcbxtools convert …– logs stay at INFO. Apply settings first (or reconfigure afterward) so the saved verbosity takes effect.- if args.command == "config": - logger = setup_logging( - getattr(args, "verbose", False), getattr(args, "silent", False) - ) - else: - logger = setup_logging(False, False) - # Apply global settings from saved configuration (CLI args take precedence) - args = apply_global_settings(args, logger) + # Apply global settings before configuring logging so persisted verbosity is respected + args = apply_global_settings(args) + logger = setup_logging( + getattr(args, "verbose", False), getattr(args, "silent", False) + )
1087-1104: Config command overwrites existing settingsThis branch starts from an empty dict, so updating one flag wipes any others already stored. Repro:
cbxtools config --threads 6, thencbxtools config --verbose→settings.jsonnow only containsverbose. Load the current settings, merge in the new values, and only write when something actually changed.- settings = {} + settings, _ = load_global_settings() + settings = dict(settings) + updated = False if hasattr(args, "verbose") and args.verbose: - settings["verbose"] = True + if settings.get("verbose") is not True: + settings["verbose"] = True + updated = True if hasattr(args, "silent") and args.silent: - settings["silent"] = True + if settings.get("silent") is not True: + settings["silent"] = True + updated = True if hasattr(args, "threads") and args.threads is not None: - settings["threads"] = args.threads - if settings: + if settings.get("threads") != args.threads: + settings["threads"] = args.threads + updated = True + if updated: logger.info("Saving global settings...") success, error = save_global_settings(settings) if success: logger.info("✓ Global settings saved successfully") return 0 else: logger.error(f"Failed to save global settings: {error}") return 1 else: logger.info( "No settings to save. Use --verbose, --silent, or --threads to set global options." ) logger.info("Note: This command now explicitly saves global settings when options are provided.") return 0
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Updated the `apply_global_settings` function to handle thread settings more robustly, ensuring defaults are applied correctly. - Enhanced the `handle_config_command` function to load global settings and maintain a copy for modifications, improving configuration handling. - Adjusted the main function to apply global settings before setting up logging, ensuring that verbosity and silence settings are respected during command execution. These changes enhance the overall usability and maintainability of the CLI module while preserving existing functionality.
…into cli-restructure
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cbxtools/cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cbxtools/cli.py
📄 CodeRabbit inference engine (AGENTS.md)
cbxtools/cli.py: CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Files:
cbxtools/cli.py
cbxtools/@(near_greyscale_scan|cli).py
📄 CodeRabbit inference engine (AGENTS.md)
For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Files:
cbxtools/cli.py
🧠 Learnings (5)
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Implement preset application via apply_preset_with_overrides() to resolve CLI args over presets over defaults
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Expose new preprocessing options through the preset system so they can be configured per preset
Applied to files:
cbxtools/cli.py
🧬 Code graph analysis (1)
cbxtools/cli.py (5)
cbxtools/core/file_processor.py (1)
find_processable_items(257-291)cbxtools/presets.py (3)
apply_preset_with_overrides(216-255)list_available_presets(89-92)import_presets_from_file(139-187)cbxtools/stats_tracker.py (3)
StatsTracker(14-104)add_run(55-75)print_lifetime_stats(159-173)cbxtools/utils.py (1)
setup_logging(12-26)cbxtools/conversion.py (4)
error(248-248)warning(247-247)info(246-246)process_archive_files(532-652)
🪛 Ruff (0.13.1)
cbxtools/cli.py
863-863: Expected an expression or a '}'
(invalid-syntax)
879-879: Expected ',', found name
(invalid-syntax)
879-879: Expected ',', found '='
(invalid-syntax)
886-886: Expected ',', found name
(invalid-syntax)
886-886: Expected ',', found '='
(invalid-syntax)
886-887: Expected '}', found newline
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Removed redundant line that was causing parser defaults to overwrite preset settings. - Ensured that the correct preset parameters are applied when specified, improving the reliability of the conversion command. This change enhances the functionality of the CLI module by preserving user-defined presets during command execution.
…into cli-restructure
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cbxtools/cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cbxtools/cli.py
📄 CodeRabbit inference engine (AGENTS.md)
cbxtools/cli.py: CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Files:
cbxtools/cli.py
cbxtools/@(near_greyscale_scan|cli).py
📄 CodeRabbit inference engine (AGENTS.md)
For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Files:
cbxtools/cli.py
🧠 Learnings (5)
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : CLI must perform dependency checks and offer installation via check_and_install_dependencies()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/cli.py : Surface new preprocessing or analysis options as CLI arguments in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/@(near_greyscale_scan|cli).py : For new analysis modes, extend near_greyscale_scan.py and add corresponding CLI options in parse_arguments()
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Implement preset application via apply_preset_with_overrides() to resolve CLI args over presets over defaults
Applied to files:
cbxtools/cli.py
📚 Learning: 2025-09-26T03:27:42.084Z
Learnt from: CR
PR: Archov/CbxTools#0
File: AGENTS.md:0-0
Timestamp: 2025-09-26T03:27:42.084Z
Learning: Applies to cbxtools/presets.py : Expose new preprocessing options through the preset system so they can be configured per preset
Applied to files:
cbxtools/cli.py
🧬 Code graph analysis (1)
cbxtools/cli.py (5)
cbxtools/core/file_processor.py (1)
find_processable_items(257-291)cbxtools/presets.py (3)
apply_preset_with_overrides(216-255)list_available_presets(89-92)import_presets_from_file(139-187)cbxtools/stats_tracker.py (3)
StatsTracker(14-104)add_run(55-75)print_lifetime_stats(159-173)cbxtools/utils.py (1)
setup_logging(12-26)cbxtools/conversion.py (4)
error(248-248)warning(247-247)info(246-246)process_archive_files(532-652)
🪛 Ruff (0.13.1)
cbxtools/cli.py
72-72: Consider moving this statement to an else block
(TRY300)
302-302: Do not catch blind exception: Exception
(BLE001)
367-367: subprocess call: check for execution of untrusted input
(S603)
378-378: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
379-379: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
380-380: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
390-390: subprocess call: check for execution of untrusted input
(S603)
414-414: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
416-416: Do not catch blind exception: Exception
(BLE001)
417-417: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
424-424: Unused method argument: usage
(ARG002)
424-424: Unused method argument: actions
(ARG002)
424-424: Unused method argument: groups
(ARG002)
893-893: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
902-902: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
917-917: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
935-935: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
971-971: Consider moving this statement to an else block
(TRY300)
973-973: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
977-977: Unused function argument: args
(ARG001)
977-977: Unused function argument: stats_tracker
(ARG001)
983-983: Unused function argument: stats_tracker
(ARG001)
995-995: Unused function argument: stats_tracker
(ARG001)
1005-1005: Unused function argument: args
(ARG001)
1011-1011: Unused function argument: args
(ARG001)
1017-1017: Unused function argument: args
(ARG001)
1058-1058: Do not catch blind exception: Exception
(BLE001)
1059-1059: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1079-1079: Unused function argument: stats_tracker
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
| unset_threads = not hasattr(args, "threads") or args.threads in (None, 0) | ||
| if unset_threads and "threads" in settings: | ||
| args.threads = settings["threads"] |
There was a problem hiding this comment.
Cannot override saved threads preference with --threads 0
After cbxtools config --threads N, any later attempt to run cbxtools convert … --threads 0 should force the auto-detect behaviour. Instead, this block treats 0 as “unset”, so apply_global_settings rewrites the explicit CLI value back to the persisted one. In practice, once a non-zero thread count is saved, the user cannot temporarily return to auto mode without editing the settings file—contrary to the intent of “don’t override explicit args”.
Please switch to using None as the sentinel for “not provided”, adjust the parser defaults accordingly, and normalise to 0 immediately before the conversion pipeline so downstream code still receives an integer.
@@ def apply_global_settings(args, logger=None):
- unset_threads = not hasattr(args, "threads") or args.threads in (None, 0)
+ unset_threads = not hasattr(args, "threads") or args.threads is None
@@ def parse_arguments():
func=handle_convert_command,
verbose=False,
silent=False,
- threads=0,
+ threads=None,
keep_originals=False,
no_cbz=False,
zip_compression=6,
@@ def _add_convert_arguments(parser):
general_group.add_argument(
"--threads",
type=int,
- default=0,
+ default=None,
help="Number of worker threads to use (0 = auto-detect)",
)
@@ def handle_convert_command(args, logger, stats_tracker=None):
- # Process archives
+ # Ensure downstream code always receives an integer thread count
+ if getattr(args, "threads", None) is None:
+ args.threads = 0
+
+ # Process archivesAlso applies to: 472-480, 642-646, 938-948
🤖 Prompt for AI Agents
In cbxtools/cli.py around lines 89-91 (and similarly at 472-480, 642-646,
938-948), the current logic treats CLI value 0 as "unset" and therefore
overwrites an explicit --threads 0 with the saved settings; change the sentinel
so the argument parser uses None as the default for threads (so unset_threads is
simply args.threads is None), update these checks to only consider None as not
provided, and then normalize None->0 immediately before entering the conversion
pipeline so downstream code still receives an integer while allowing users to
explicitly pass --threads 0 to force auto-detect. Ensure the settings write/read
paths remain compatible with integer values and update any related help text to
state that 0 means auto-detect.
1 participant
convert,watch,debug,scan,config, andstats.Summary by Sourcery
Restructure the CLI to use a subcommand-based interface, add persistent global settings management via a JSON config, and enhance dependency management by parsing requirements.txt for dynamic handling of required and optional packages while improving logging, error handling, and help formatting.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation