Refactor commander and Click support #49
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new ClickCommander, which generates CLI applications using the Click framework. It also refactors the existing Commander class to support both Click and Typer frameworks. The changes include adding a ClickParser class to handle Click-specific parsing, updating the Commander class to manage groups and subcommands, and modifying the CLI generation process to accommodate both frameworks. Class diagram showing the updated Commander hierarchy and new Click classesclassDiagram
class Commander {
<<abstract>>
+generate_cli()
+add_base_imports()*
+add_base_cli()*
+add_root_command()*
+add_sub_command()*
+add_main_block()*
+define_groups()*
}
class ClickCommander {
+add_base_imports()
+add_base_cli()
+add_root_command()
+add_sub_command()
+add_main_block()
+define_groups()
}
class ClickParser {
+parse_run_block()
+parse_command_run()
+get_param_names()
+parse_params()
+get_command_func_name()
+get_parsed_command_name()
}
class BaseGroup {
+name: str
+short_name: str
+parent_group: BaseGroup
+commands: list[Command]
+help: str
+var_name()
+is_root()
}
Commander <|-- ClickCommander
ClickCommander --> ClickParser
ClickCommander --> BaseGroup
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughArrr, matey! This pull request be bringin' a treasure trove of changes to the CLI framework! We've got a complete overhaul of the Changes
Sequence DiagramsequenceDiagram
participant Commander
participant Groups
participant BaseGroup
participant Command
Commander->>Groups: Initialize groups
Groups->>BaseGroup: Create base groups
BaseGroup-->>Groups: Register groups
Commander->>Command: Add commands to groups
Groups->>BaseGroup: Process commands
BaseGroup-->>Commander: Confirm group structure
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jaykv - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| return "\n".join(decorators) | ||
|
|
||
| def _parse_simple_param(self, param: SimpleCommandParam) -> str: |
There was a problem hiding this comment.
issue: Add error handling for malformed param_spec parsing
The type_parts split operation could raise an IndexError if param_spec is malformed. Consider adding try/except handling and validation of the param_spec format.
| else: | ||
| option_parts.append(f"default={param.default}") | ||
|
|
||
| if param.help: |
There was a problem hiding this comment.
issue (bug_risk): Escape single quotes in help text
Help text containing single quotes will cause syntax errors. Consider escaping single quotes in the help text.
| norm_script = script.to_script() if isinstance(script, RunBlockList) else script.root | ||
| if not isinstance(norm_script, str): | ||
| raise ValueError(f"Invalid script type: {type(norm_script)}") | ||
| parsed_script: str = transform_bash(norm_script).strip() |
There was a problem hiding this comment.
issue: Add error handling for transform_bash failures
Add try/except handling around transform_bash call to gracefully handle transformation failures.
| param_name = option_match.group(1).lstrip("-").replace("-", "_") | ||
| params += f"{param_name}, " | ||
| elif argument_match: | ||
| param_name = argument_match.group(1).replace("-", "_") |
There was a problem hiding this comment.
issue (code-quality): Replace m.group(x) with m[x] for re.Match objects [×2] (use-getitem-for-re-match-groups)
| required = param_spec.endswith("!") | ||
|
|
||
| # Determine if option or argument based on name prefix | ||
| if name.startswith("-"): |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Remove unnecessary else after guard condition (
remove-unnecessary-else) - Extract code out into method (
extract-method)
coderabbitai
bot
changed the title
jaykv
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 8257e30 in 2 minutes and 17 seconds
More details
- Looked at
935lines of code in8files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. cliffy/commander.py:37
- Draft comment:
Using a mutable default value forcommands. Usedefault_factory=listto avoid shared state between instances. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The BaseGroup class is new in this PR, so this is commenting on changed code. The issue is real and could cause bugs. However, since this is using Pydantic's BaseModel, the default list behavior may be different than standard Python classes. Looking at Pydantic docs, it actually handles mutable defaults correctly internally, so this isn't actually an issue.
I should verify that Pydantic BaseModel definitely handles this correctly in all cases. There could be edge cases I'm not considering.
Pydantic's documentation explicitly states that it handles mutable defaults safely by creating new instances. This is a core feature of Pydantic.
The comment should be deleted because it raises a concern that isn't actually an issue when using Pydantic models, which handle mutable defaults correctly.
2. cliffy/commander.py:52
- Draft comment:
Using a mutable default value forroot. Usedefault_factory=dictto avoid shared state between instances. - Reason this comment was not posted:
Marked as duplicate.
3. cliffy/commander.py:69
- Draft comment:
Handle the case wherefull_pathis an empty string to avoid unexpected behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests adding explicit empty string handling, but this isn't necessary because: 1) The method is only called internally from process_command, 2) process_command only calls it when there's a "." in command.name, 3) The Pydantic model validation would catch empty names anyway. Adding explicit empty string handling would be redundant.
I could be wrong about the validation - maybe there are edge cases where an empty string could slip through. And defensive programming is generally good practice.
While defensive programming is good, in this case the validation is already handled by multiple layers - the command name validation, the "." checks, and the Pydantic model. Adding another check would be redundant.
The comment should be deleted since empty string handling is already covered by existing validation and the method is only called in controlled internal contexts.
4. cliffy/commander.py:71
- Draft comment:
Check iffull_pathis already inself.rootbefore adding it to avoid overwriting existing groups. - Reason this comment was not posted:
Marked as duplicate.
5. cliffy/commander.py:70
- Draft comment:
Handle the case wherefull_pathis an empty string to avoid unexpected behavior. - Reason this comment was not posted:
Marked as duplicate.
6. examples/click_hello.yaml:1
- Draft comment:
Use mkdocs material admonitions for better readability in documentation examples. For instance, use the following format for examples:
!!! example "Title"
This is an example of a mkdocs material admonitions. You can use code blocks here too.
This applies to both click_hello.yaml and nested.yaml examples.
- Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_AApUflOGlgWYf45V
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![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)
cliffy/commanders/click.py (1)
190-209: 'parse_params' method organizes the param decorators.Be mindful if
Global Paramsmight overshadow local ones in some edge scenario, but logic seems good.cliffy/commander.py (1)
136-155: '_merge_command_template' merges commands with a template block.Arrr, take care of merges overshadowing manually set fields if that’s not intended.
cliffy/commanders/typer.py (1)
101-107: Hoist the sails for group definitions!
'define_groups' method neatly arranges each group, though watch for multiline help in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cliffy/commander.py(6 hunks)cliffy/commanders/click.py(1 hunks)cliffy/commanders/typer.py(2 hunks)cliffy/manifest.py(1 hunks)cliffy/transformer.py(2 hunks)examples/click_hello.yaml(1 hunks)examples/nested.yaml(1 hunks)tests/test_commander.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/nested.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeBeaver AI
🔇 Additional comments (74)
cliffy/commanders/click.py (28)
2-2: Arrr, no concerns here, matey!Ye be importin' the
Uniontype fromtyping, which be perfectly fine.
5-5: Ahoy, 'Commander' and 'BaseGroup' import be a neat approach!Keep a weather eye out for consistent usage with your newly minted classes.
9-10: Steerin’ the script transformation to safe waters!Ye be importin'
transform_bash; all seems well, but be mindful of performance if scripts grow large.
11-18: Avast! The 'CLIManifest' and param classes be loaded.All these imports be fair and square. Just be sure none be left adrift if unused.
21-21: A new 'ClickCommander' class, me hearty!This be the anchor of the new approach, no immediate issues spotted.
23-23: Docstring be clear as the horizon on a calm sea!Nice clarity mentionin' the usage of Click.
25-30: Constructor sets sail with 'ClickParser' and base imports!All looks like plain sailing. The usage of
rich_clickis interesting—just mind licensing if it’s external.
33-36: Arrr, your 'add_base_imports' sets a sweet introduction.The dynamic injection of imports be well done.
37-58: 'add_base_cli' method hoists the mainsail with grouped context settings!Well structured and prepared for optional aliases. Good usage of an exit after showin’ aliases.
60-69: Click group definition gets a well-fitting coat of paint.All be spick and span. Mind you confirm the
'--aliases'option doesn't conflict with user-submitted options.
76-79: The 'define_groups' method outlines your group creation.Clear and purposeful, me hearty.
80-85: Conditional skip for root group checks, well done!Ye mark root groups to avoid accidental reuse. The help text replacement be short but sweet.
89-90: 'add_group' gives each group a place on the deck.Linkin' parent group gracefully.
92-92: Small addition to the CLI string.No trouble spotted.
95-95: 'add_root_command' sets the stage for commands.No immediate concerns, matey.
96-99: Guard check for run block.If no run block, we skip. Fair enough—keeps the ship uncluttered.
100-109: Root commands capturing aliases, arrr!This be a robust approach to handle multiple sets of commands and their aliases.
111-119: Alias creation in root commands.Bundling them with hidden attribute is a cunning plan.
120-120: Sub-command aggregator for group.A single line that anchors the method. Looks well set.
121-123: Naming and help for commands.All be consistent with the rest o’ the code.
126-131: Sub-command method’s final flourish.Neatly sets the command run block. No issues, me hearty.
133-139: Alias for sub-commands, hidden from the main listing.Clever trick, well used.
142-143: Addin' the main block to the CLI.'if name == "main":' suits a direct execution.
144-146: Invoke the 'cli' group.A neat piece for direct script usage.
149-188: 'ClickParser' constructor, parse_run_block, parse_command_run—ye do be handling script transformations thoroughly.All be logically cohesive. Just watch for potential corner cases if script gets monstrous.
210-252: '_parse_simple_param' yields clarity for arguments vs. options.Mind the distinction between required vs. default—some combos can cause confusion if both exist.
253-276: '_build_click_option' robustly handles short flags, types, defaults, and help.Spot on, me hearty!
278-289: 'get_command_func_name', 'get_parsed_command_name', and 'to_args' keep naming in shipshape.They appear to handle edge cases adequately.
cliffy/commander.py (25)
3-4: Marking 'Commander' as an ABC: Shiver me timbers, a bold move!This clarifies it be an abstract base class.
7-7: Importing 'defaultdict'—smart to handle grouped lists or dictionaries, matey.Just be certain not to introduce circular references.
23-23: Sizable import block, but the usage is quite cohesive.No further suggestions.
33-33: 'BaseGroup' class sets the foundation for the new group structure.A good pivot from older group classes, me hearty.
35-37: Short name, optional parent group, and list of commands—yo-ho-ho, a flexible group design.Ensure no cyclical parent references creep in.
40-47: 'var_name' returns a valid Python variable, 'is_root' checks the anchor.Nifty approach to keep group naming consistent.
49-52: 'Groups' container easing iteration and retrieval.Surely better than a plain dictionary.
53-68: Iterator, items, values, indexing, length: all hands on deck for grouping convenience.All typical dictionary-like expansions. Approved.
69-90: Adding groups by full path, bravely forging parent group references.Watch for missing sub-groups if multiple dots appear. Looks well-structured though.
91-99: 'process_command' ensures commands find their rightful group.No issues be spotted.
100-100: Commander class decorated with ABC docstring.Aye, no trouble.
113-113: Double definition of 'groups' in slots might be redundant.Check if 'groups' is repeated by oversight. Could lead to confusion.
121-122: Initializing 'groups' as 'Groups()'—the new aggregator.Central place for group logic. All good.
126-126: Assigning default names for unnamed commands.This autopopulation is a handy fallback.
127-129: 'aliases_by_commands' is a defaultdict of lists.Neat approach. Keep an eye that no extraneous aliases slip in.
158-158: 'build_groups' sets the skeleton.No issues, me hearty.
164-165: Merging user-supplied template on each command.Practical approach.
166-171: Handling '|' in command name for aliases.Nice trick. Keep your doc references updated so others know.
[approve]
172-180: Finish group building by checking if a command is a script-less group.Sets group help if found. Crisp logic.
181-187: 'add_subcommands' loops through groups, skip root, add group.Then subcommands follow. Very clean.
202-202: 'add_root_commands': roots get their rightful place.Sensible design.
239-239: 'add_command' for nested commands: a direct approach.No scurvy to be found.
246-248: 'add_greedy_commands' for placeholders like '(*)'.Clever tactic for multi-group expansions.
251-258: 'add_lazy_command': the real wizard behind the scenes.Bravely manipulates placeholders in commands, keep watch for performance if many placeholders exist.
323-350: Abstract methods for each step: define_groups, add_group, add_base_imports, etc.This sets up an interface for future expansions.
cliffy/transformer.py (2)
10-10: Import of 'ClickCommander' gives us a new route for generation.No red flags, me hearty.
47-48: Dynamic commander class selection with 'use_click' field.Brilliant design choice—lets us swap frameworks swiftly.
cliffy/commanders/typer.py (8)
3-3: Arr, the imports be lookin' shipshape!
No conflicts or naming woes are spotted after hooking inBaseGroup.
87-87: A fine tweak to the help text, me hearty!
Usingempty_or_helpfor conditionally showin' help text is clear as crystal.
93-93: Command registration sets sail smoothly!
No hidden reefs found incli.commandusage.
110-114: Smart group addition, matey!
Proper nesting logic withparent_groupis well-anchored.
116-117: Sub-command docstring be quite helpful!
The purpose and usage are plain to see.
122-123: Conditionally attachin' help text be wise.
This approach stays consistent with the root commands.
128-128: Command creation sails forth!
All parameters match the new approach.
133-133: Alias commands be set with no mutiny!
No scurvy found; aliases be hidden properly.tests/test_commander.py (7)
65-69: Aye, group additions be tested!
Verifications ofgroup1andgroup2commands are well-labeled.
73-73: Avast, the group count check be correct!
Ensures the manifest translation forms the correct number of groups.
76-77: Aar, help text tested for clarity!
Properly ensures each group’s help is captured.
153-155: Param expansions run without sinking!
The assertion for replaced placeholders appears accurate.
169-170: Command retrieval by iteration is sound!
Fetching commands from the group dictionary is neat and tidy.
181-183: Groups be shaped for a complex voyage!
Introducin'group1andgroup2with clear help messages.
190-197: Detailed checks for nested subgroups, arr!
The hierarchical logic and help text validations be thorough.cliffy/manifest.py (1)
253-253:use_clickhoists a new flag, me hearties!
Now the CLI can set sail with Click or Typer.examples/click_hello.yaml (3)
1-3: This new manifest be stowin’ a Click-based CLI, argh!
name,version, anduse_clickoptions anchor the plan for a clickable CLI.
5-7: Help message and commands starboard!
Defining a simple 'hello' command with an alias shows promise for a friendly interface.
9-9: Empty line—no troubles, matey!
This empty space does not hide any cursed code.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 67fbc5c in 29 seconds
More details
- Looked at
63lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. cliffy/loader.py:38
- Draft comment:
Ensurereplace('-', '_')is consistently applied across the codebase for CLI names to avoid potential import errors. This is already done here and inget_cli_script, but verify other parts of the codebase. - Reason this comment was not posted:
Confidence changes required:50%
The code incliffy/loader.pyusesreplace('-', '_')in multiple places to ensure CLI names are valid Python identifiers. This is a good practice, but it should be consistently applied across the codebase.
2. tests/test_examples.py:51
- Draft comment:
The test cases fornested-cliare well-defined and cover the expected outputs for the commands. Ensure that any future changes to the command structure are reflected in these tests. - Reason this comment was not posted:
Confidence changes required:20%
The test filetests/test_examples.pyincludes a test fornested-cli, which is a new feature. The test cases seem to cover the expected outputs for the commands defined inexamples/nested-cli.yaml. This is a good practice to ensure new features are tested.
3. examples/nested-cli.yaml:1
- Draft comment:
Consider using mkdocs material admonitions for examples to improve readability. For instance, use the following format:
!!! example "Nested CLI Example"
name: nested-cli
version: 0.1.0
help: Demo nested command structure
commands:
group1.subgroup.command1: $ echo "hello"
group1.subgroup.command2: $ echo "world"
group2.command3: $ echo "foo"- Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_npYoxnMHe9zA7FnU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_examples.py (1)
51-55: Shiver me timbers, yer nested subcommand tests be grand!The coverage for multiple subcommands be impressive. Consider testin' unrecognized commands or error-handling flows to keep mutiny in check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cliffy/loader.py(2 hunks)examples/nested-cli.yaml(1 hunks)tests/test_examples.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeBeaver AI
🔇 Additional comments (5)
cliffy/loader.py (2)
38-38: Arr, starboard rename be the right course, me hearty!Changin' hyphens to underscores keeps Python module scurvy at bay. Tidy work, mate!
48-48: Ahoy, consistency in yer import path is shipshape!Ye be replacin' hyphens with underscores in both file path and import statement, maintainin' harmony across the deck, argh!
tests/test_examples.py (1)
21-22: Avast! Ye've added nested-cli to yer builds, mate.This new entry helps test the nested structure feature thoroughly. Set sail with confidence!
examples/nested-cli.yaml (2)
3-3: Arr, the new name 'nested-cli' be on point!It aligns well with yer hyphen-to-underscore approach in the loader, savvy?
5-5: By the powers! This 'help' field adds clarity, matey!Settin' up user-friendly info fosters smoother sailin' for all hands on deck.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5c5c0a5 in 19 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. cliffy/commander.py:113
- Draft comment:
The 'groups' attribute is initialized in the constructor but removed from slots. Ensure consistency between slots and attributes used in the class. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_waI9jqMmaulA9ojo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on de2a003 in 58 seconds
More details
- Looked at
847lines of code in22files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. Makefile:42
- Draft comment:
Ensure thatkeyringis necessary for CLI generation. If not, consider removing it from thegenerate-clisandgenerate-cleanuptargets to avoid unnecessary dependencies. - Reason this comment was not posted:
Confidence changes required:50%
Thegenerate-clisandgenerate-cleanuptargets in the Makefile now includekeyringin the list of packages to install and uninstall. This is a good practice to ensure that all dependencies are managed properly. However, it's important to ensure thatkeyringis actually used in the CLI generation process, otherwise, it might be an unnecessary dependency.
2. cliffy/commander.py:88
- Draft comment:
Ensure thatadd_command_to_groupcorrectly handles commands without a dot in their name, as these should be added to the root group instead of creating a new group. - Reason this comment was not posted:
Confidence changes required:50%
Theadd_command_to_groupmethod in theGroupsclass assumes that the command name will always contain a dot to determine the group name. This might not be the case for root commands, which could lead to incorrect group assignments.
3. cliffy/commander.py:197
- Draft comment:
Good use ofself.root_group.commandsto handle root commands separately. This keeps the logic clear and organized. - Reason this comment was not posted:
Confidence changes required:0%
Theadd_root_commandsmethod in theCommanderclass iterates overself.root_group.commandsto add root commands. This is a clear and efficient way to handle root commands separately from grouped commands.
4. cliffy/commanders/click.py:77
- Draft comment:
Ensure that all groups are correctly initialized and populated before callingdefine_groupsto avoid potential issues with undefined groups. - Reason this comment was not posted:
Confidence changes required:50%
Thedefine_groupsmethod inClickCommanderiterates overself.groups.values()to define groups. This is a straightforward approach, but it assumes that all groups have been correctly initialized and populated.
5. cliffy/manifest.py:253
- Draft comment:
Good use of default values for fields inCLIManifestto ensure they are always initialized. - Reason this comment was not posted:
Confidence changes required:0%
Theuse_clickfield inCLIManifesthas been updated to use a default value. This is a good practice to ensure that the field always has a value, even if not explicitly set by the user.
6. examples/generated/aiprompt.py:1
- Draft comment:
Use mkdocs material admonitions for examples. For instance, use the following format for examples:
!!! example "Generated aiprompt"
This is an example of a mkdocs material admonitions. You can use code blocks here too.
- Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_tuyEkJ1Monz8rk8Q
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
examples/nested-cli.yaml (1)
10-10: Arrr! Missing newline at the end of the file, ye scallywag!Add a newline character at the end of the file to comply with YAML best practices.
command4: $ echo "bar" +🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
Makefile (1)
42-42: Shiver me timbers! The keyring package be needin' a version pin!All other packages have their versions pinned. For consistency and to prevent future compatibility issues, consider pinning the keyring package version.
-pip install requests "six<1.0.0" rich keyring +pip install requests "six<1.0.0" rich "keyring==24.3.0"examples/cliffy_schema.json (1)
300-305: Aye! Add an example for use_click property, ye landlubber!Ye schema be needin' an example to show how to use the Click framework, matey!
"use_click": { "default": false, "description": "Generate CLI to Click instead of Typer", "title": "Use Click", - "type": "boolean" + "type": "boolean", + "examples": [ + { + "use_click": true, + "name": "my-cli", + "version": "1.0.0" + } + ] },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
examples/generated/aiprompt.pyis excluded by!**/generated/**examples/generated/clickhello.pyis excluded by!**/generated/**examples/generated/db.pyis excluded by!**/generated/**examples/generated/environ.pyis excluded by!**/generated/**examples/generated/hello.pyis excluded by!**/generated/**examples/generated/nested_cli.pyis excluded by!**/generated/**examples/generated/penv.pyis excluded by!**/generated/**examples/generated/pydev.pyis excluded by!**/generated/**examples/generated/requires.pyis excluded by!**/generated/**examples/generated/taskmaster.pyis excluded by!**/generated/**examples/generated/template.pyis excluded by!**/generated/**examples/generated/todo.pyis excluded by!**/generated/**examples/generated/town.pyis excluded by!**/generated/**
📒 Files selected for processing (9)
Makefile(1 hunks)cliffy/commander.py(6 hunks)cliffy/commanders/click.py(1 hunks)cliffy/commanders/typer.py(2 hunks)cliffy/manifest.py(1 hunks)examples/cliffy_schema.json(1 hunks)examples/nested-cli.yaml(1 hunks)tests/test_commander.py(3 hunks)tests/test_examples.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cliffy/manifest.py
🧰 Additional context used
🪛 yamllint (1.35.1)
examples/nested-cli.yaml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeBeaver AI
🔇 Additional comments (12)
examples/nested-cli.yaml (1)
3-5: Ahoy! Nice and clear command structure, matey!Ye've done a fine job with the command structure and the help text be crystal clear!
cliffy/commanders/typer.py (3)
87-88: Aye! Smart move with the help text handling, ye code wizard!The
empty_or_helpvariable be making the code more DRY and easier to maintain. Well done!Also applies to: 120-121
101-105: Arrr! A fine addition with the define_groups method!The new method provides a clean way to handle group definitions. The implementation be ship-shape!
108-112: Ye've done well with the group nesting, matey!The updated
add_groupmethod with proper parent-child relationships makes the command hierarchy crystal clear.tests/test_examples.py (1)
51-56: Yo ho ho! Excellent test coverage for the nested commands!The test cases be covering all the commands in the nested structure, including the new command4. That's the way to prevent bugs from sneaking aboard!
tests/test_commander.py (4)
65-69: Aye! The group structure be well-organized!The command hierarchy with help text makes the structure crystal clear. Good job, matey!
153-155: Smart move with the type hints, ye code sailor!Adding type hints to suppress warnings makes the code more maintainable and type-safe.
169-170: Arrr! A more Pythonic approach to command iteration!Using
next(iter())instead of list comprehension be more efficient and cleaner.
181-197: Shiver me timbers! That's some fine group hierarchy testing!The test coverage for complex command hierarchies be thorough and well-structured. The parent-child relationships and help text verification be spot on!
cliffy/commanders/click.py (3)
155-155: Arrr! Add error handling for transform_bash, ye scurvy dog!Ye be needin' to wrap that transform_bash call in a try/except block, lest ye hit rough seas!
- parsed_script: str = transform_bash(norm_script).strip() + try: + parsed_script: str = transform_bash(norm_script).strip() + except Exception as e: + raise ValueError(f"Failed to transform bash script: {e}") from e
270-271: Shiver me timbers! Escape them single quotes in help text!Ye help text be needin' some escapin' of single quotes, or ye'll be walkin' the plank with syntax errors!
- option_parts.append(f"help='{param.help}'") + option_parts.append(f"help='{param.help.replace(\"'\", \"\\'\")}'")
208-217: Avast! Secure yer param_spec parsing, matey!Ye type_parts split operation could raise an IndexError if param_spec be malformed. Add some safety checks!
- type_parts = param_spec.split("=") - param_type = type_parts[0].rstrip("!") - default_value = type_parts[1] if len(type_parts) > 1 else None + try: + type_parts = param_spec.split("=") + param_type = type_parts[0].rstrip("!") + default_value = type_parts[1] if len(type_parts) > 1 else None + except IndexError: + raise ValueError(f"Invalid param_spec format: {param_spec}")
| template = self.manifest.command_templates.get(command.template) | ||
| if not template: | ||
| raise ValueError(f"Template {command.template} undefined in command_templates") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Arrr! Add more validation for ye template existence!
Ye be needin' to validate the template name before tryin' to fetch it, or ye might hit a reef!
+ if not command.template.strip():
+ raise ValueError("Empty template name provided")
+
template = self.manifest.command_templates.get(command.template)
if not template:
- raise ValueError(f"Template {command.template} undefined in command_templates")
+ raise ValueError(
+ f"Template '{command.template}' not found in command_templates. "
+ f"Available templates: {list(self.manifest.command_templates.keys())}"
+ )📝 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.
| template = self.manifest.command_templates.get(command.template) | |
| if not template: | |
| raise ValueError(f"Template {command.template} undefined in command_templates") | |
| if not command.template.strip(): | |
| raise ValueError("Empty template name provided") | |
| template = self.manifest.command_templates.get(command.template) | |
| if not template: | |
| raise ValueError( | |
| f"Template '{command.template}' not found in command_templates. " | |
| f"Available templates: {list(self.manifest.command_templates.keys())}" | |
| ) |
| def add_group_by_full_path(self, full_path: str) -> str: | ||
| if "." not in full_path: | ||
| self.root[full_path] = BaseGroup(name=full_path, short_name=full_path, parent_group=None) | ||
| return full_path | ||
|
|
||
| group_name, _ = full_path.rsplit(".", 1) | ||
| if "." in group_name: | ||
| parent_group_name, short_name = group_name.rsplit(".", 1) | ||
| if parent_group_name in self.root: | ||
| parent_group = self.root[parent_group_name] | ||
| else: | ||
| parent_group = self.root[self.add_group_by_full_path(parent_group_name)] | ||
| else: | ||
| short_name = group_name | ||
| parent_group = None | ||
|
|
||
| if group_name in self.root: | ||
| return group_name | ||
|
|
||
| self.root[group_name] = BaseGroup(name=group_name, short_name=short_name, parent_group=parent_group) | ||
| return group_name | ||
|
|
||
| def add_command_to_group(self, command: Command) -> None: | ||
| group_name = self.add_group_by_full_path(command.name) | ||
| self.root[group_name].commands.append(command) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ye need better error handling in add_group_by_full_path, cap'n!
Add some validation to prevent invalid group paths from sinkin' yer ship!
def add_group_by_full_path(self, full_path: str) -> str:
+ if not full_path or not isinstance(full_path, str):
+ raise ValueError(f"Invalid group path: {full_path}")
+
if "." not in full_path:
self.root[full_path] = BaseGroup(name=full_path, short_name=full_path, parent_group=None)
return full_path📝 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.
| def add_group_by_full_path(self, full_path: str) -> str: | |
| if "." not in full_path: | |
| self.root[full_path] = BaseGroup(name=full_path, short_name=full_path, parent_group=None) | |
| return full_path | |
| group_name, _ = full_path.rsplit(".", 1) | |
| if "." in group_name: | |
| parent_group_name, short_name = group_name.rsplit(".", 1) | |
| if parent_group_name in self.root: | |
| parent_group = self.root[parent_group_name] | |
| else: | |
| parent_group = self.root[self.add_group_by_full_path(parent_group_name)] | |
| else: | |
| short_name = group_name | |
| parent_group = None | |
| if group_name in self.root: | |
| return group_name | |
| self.root[group_name] = BaseGroup(name=group_name, short_name=short_name, parent_group=parent_group) | |
| return group_name | |
| def add_command_to_group(self, command: Command) -> None: | |
| group_name = self.add_group_by_full_path(command.name) | |
| self.root[group_name].commands.append(command) | |
| def add_group_by_full_path(self, full_path: str) -> str: | |
| if not full_path or not isinstance(full_path, str): | |
| raise ValueError(f"Invalid group path: {full_path}") | |
| if "." not in full_path: | |
| self.root[full_path] = BaseGroup(name=full_path, short_name=full_path, parent_group=None) | |
| return full_path | |
| group_name, _ = full_path.rsplit(".", 1) | |
| if "." in group_name: | |
| parent_group_name, short_name = group_name.rsplit(".", 1) | |
| if parent_group_name in self.root: | |
| parent_group = self.root[parent_group_name] | |
| else: | |
| parent_group = self.root[self.add_group_by_full_path(parent_group_name)] | |
| else: | |
| short_name = group_name | |
| parent_group = None | |
| if group_name in self.root: | |
| return group_name | |
| self.root[group_name] = BaseGroup(name=group_name, short_name=short_name, parent_group=parent_group) | |
| return group_name | |
| def add_command_to_group(self, command: Command) -> None: | |
| group_name = self.add_group_by_full_path(command.name) | |
| self.root[group_name].commands.append(command) |
Summary by Sourcery
Implement CLI generation using Click.
New Features:
Tests:
Important
Refactor CLI generation to support both Click and Typer frameworks, with new examples and updated tests.
cliffy/commanders/click.py.use_clickoption inCLIManifestto switch between Typer and Click.generate_cli()incliffy/commander.pyto useClickCommanderorTyperCommanderbased onuse_click.ClickCommanderclass incliffy/commanders/click.pyfor Click-based CLI generation.Commanderclass incliffy/commander.pyto support both Click and Typer.ClickParserclass incliffy/commanders/click.pyfor parsing Click command parameters.examples/click_hello.yamlandexamples/nested-cli.yamlfor Click-based CLI examples.tests/test_commander.pyandtests/test_examples.pyto test Click and Typer CLI generation.cliffy_schema.jsonto includeuse_clickfield.Makefilefor dependency management.This description was created by
for de2a003. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
clickhello.command4undergroup2in the nested CLI example.Improvements
Tests