fix: pass category argument through to add_task properly#53
Conversation
The add_task() function hardcoded category='General' regardless of user input, and _handle_todo_command() concatenated both arguments into a single string, discarding the category. Fix: - Update add_task() to parse quoted arguments via shlex.split() - Update _handle_todo_command() to pass arguments with quotes preserved Closes TruFoundation#46
The test_run_help_prints_docstring_for_known_command test was failing because the fake kernel lacked a _import_module method, causing the module import to fail silently and fall through to the error message. Add a working _fake_import_module that uses importlib to resolve the module path, allowing run_help to read and print the docstring.
|
You replaced Update the docs, and remove any remaining references of Fix these things, and this PR is good to be merged! |
Use typer.secho with colors for warning/error/success messages and typer.echo for normal output, matching the project's existing pattern in cli.py. This ensures task feedback is visible and styled correctly in busy terminals. Addresses review feedback on TruFoundation#53.
📝 WalkthroughWalkthroughTask command functions now parse quoted task/category inputs with shlex, use typer for colorized user output, and the CLI passes quoted arguments to the task handler. A minor test formatting trailing-comma tweak was also made. ChangesTask command refactoring with colorized output and CLI integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks for the review! I've addressed your feedback:
The PR is ready for another look. Thanks! |
|
Thanks for the review! I've addressed your feedback:
PR is ready for another look. Thanks! |
![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
🧹 Nitpick comments (2)
trushell/cli.py (1)
195-195: ⚖️ Poor tradeoffConsider refactoring
add_taskto accept separate task and category parameters.The current implementation extracts quoted strings via regex, then re-wraps them in quotes before passing to
add_task, which parses them withshlex.split()again. This round-trip is unnecessary complexity.Future refactor suggestion:
def add_task(task: str, category: str = "General") -> None: ...Then call it directly:
add_task(add_match.group(1), add_match.group(2))This would eliminate the string parsing overhead and make the API clearer. However, this requires updating all call sites including the dispatcher.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trushell/cli.py` at line 195, Refactor add_task to accept two parameters (task: str, category: str = "General") instead of a single quoted string: change the signature of add_task to def add_task(task: str, category: str = "General") -> None, remove the internal shlex.split/parsing logic, and update the call site shown (replace add_task(f'"{add_match.group(1)}" "{add_match.group(2)}"') with add_task(add_match.group(1), add_match.group(2))). Also update the dispatcher and any other call sites or tests that pass a single combined quoted string to pass two separate arguments instead.trushell/commands/tasks.py (1)
23-23: ⚡ Quick winMove
shleximport to the top of the file.Importing inside the function is unconventional and can add minor overhead. PEP 8 recommends placing imports at module level.
♻️ Suggested refactor
At the top of the file (after line 3):
from typing import Callable +import shlex import typerThen remove the import from inside the function:
# Try to parse quoted arguments: "task text" "category" - import shlex try: parts = shlex.split(args)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trushell/commands/tasks.py` at line 23, Move the local "import shlex" out of the function and place it with the other module-level imports at the top of the file; then delete the in-function "import shlex" statement so the code uses the module-level shlex symbol without repeated imports. Ensure no local variable shadows "shlex" and run tests/lint to confirm the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_help_docs.py`:
- Around line 10-12: The test currently defines a helper
_fake_import_module(path: str) that converts file paths to importable module
paths but then overrides fake_kernel._import_module later, so the helper is dead
and the import-path conversion isn't being tested; restore or wire the helper
into the fake kernel by assigning fake_kernel._import_module =
_fake_import_module (or call _fake_import_module from the
fake_kernel._import_module implementation) so that when the test exercises
fake_kernel it uses the conversion logic in _fake_import_module and verifies
conversion from strings like "trushell/commands/settings.py" to a dotted import
path.
In `@trushell/commands/tasks.py`:
- Around line 22-34: The parsing treats any multi-word input as two parts
because it unconditionally uses shlex.split(args); change add_task so it only
interprets args as "task text" "category" when the user actually provided two
quoted strings: call shlex.split(args) into parts but then verify that the
original args contains explicit matching quotes for both tokens (e.g., both
tokens were quoted—check for surrounding quotes or use a simple test that the
first non-space char is a quote and there are at least two quoted segments); if
that quoted-pair condition is true, assign task_text=parts[0],
category=parts[1]; otherwise treat the entire raw args (args.strip()) as the
full task_text and set category="General". This change should be applied inside
add_task where shlex.split(args), parts, task_text and category are handled.
---
Nitpick comments:
In `@trushell/cli.py`:
- Line 195: Refactor add_task to accept two parameters (task: str, category: str
= "General") instead of a single quoted string: change the signature of add_task
to def add_task(task: str, category: str = "General") -> None, remove the
internal shlex.split/parsing logic, and update the call site shown (replace
add_task(f'"{add_match.group(1)}" "{add_match.group(2)}"') with
add_task(add_match.group(1), add_match.group(2))). Also update the dispatcher
and any other call sites or tests that pass a single combined quoted string to
pass two separate arguments instead.
In `@trushell/commands/tasks.py`:
- Line 23: Move the local "import shlex" out of the function and place it with
the other module-level imports at the top of the file; then delete the
in-function "import shlex" statement so the code uses the module-level shlex
symbol without repeated imports. Ensure no local variable shadows "shlex" and
run tests/lint to confirm the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cdfa3fd5-adf3-4a37-bcac-61794e0876f4
📒 Files selected for processing (3)
tests/test_help_docs.pytrushell/cli.pytrushell/commands/tasks.py
| def _fake_import_module(path: str): | ||
| module = importlib.import_module(path.replace("/", ".").removesuffix(".py")) | ||
| return module |
There was a problem hiding this comment.
_fake_import_module is currently dead code, so this test misses the import-path conversion contract.
Line 31 overrides fake_kernel._import_module, which bypasses the helper added on Lines 10–12/21. As written, the test no longer verifies conversion from "trushell/commands/settings.py" to an importable module path, so regressions in that behavior can slip through.
Suggested fix
def test_run_help_prints_docstring_for_known_command(monkeypatch, capsys):
def _fake_import_module(path: str):
module = importlib.import_module(path.replace("/", ".").removesuffix(".py"))
return module
@@
fake_module = SimpleNamespace(run_settings=run_settings)
- fake_kernel._import_module = lambda path: fake_module
+ monkeypatch.setattr(
+ importlib,
+ "import_module",
+ lambda module_path: (
+ fake_module if module_path == "trushell.commands.settings" else None
+ ),
+ )Also applies to: 21-21, 31-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_help_docs.py` around lines 10 - 12, The test currently defines a
helper _fake_import_module(path: str) that converts file paths to importable
module paths but then overrides fake_kernel._import_module later, so the helper
is dead and the import-path conversion isn't being tested; restore or wire the
helper into the fake kernel by assigning fake_kernel._import_module =
_fake_import_module (or call _fake_import_module from the
fake_kernel._import_module implementation) so that when the test exercises
fake_kernel it uses the conversion logic in _fake_import_module and verifies
conversion from strings like "trushell/commands/settings.py" to a dotted import
path.
- add_task now only splits on quotes when args actually starts with a quote character. Plain multi-word input like 'buy groceries' is treated as the full task text, not split into task + category. - Remove dead _fake_import_module function from test_help_docs.py (was overridden by the lambda on the next line). Addresses CodeRabbit review on TruFoundation#53 and TruFoundation#59.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
trushell/commands/tasks.py (1)
86-101: ⚡ Quick winInconsistent argument parsing in
update_task.The usage message (line 90) promises support for quoted multi-word task text:
task update <task-number> "<task>" ["<category>"], but the implementation usesargs.split(maxsplit=2)which splits on whitespace without respecting quotes.For example:
- Input:
task update 1 "new task text" "Shopping"- After
split(maxsplit=2):["1", '"new', 'task text" "Shopping"']parts[1]becomes'"new'instead of"new task text"Since this PR fixes quoted-argument parsing in
add_taskusingshlex.split(), consider applying the same approach toupdate_taskfor consistency.♻️ Proposed refactor to use shlex for proper quote handling
def update_task(args: str) -> None: """Update an existing task's text and/or category.""" - parts = args.split(maxsplit=2) + import shlex + try: + parts = shlex.split(args) + except ValueError: + typer.secho('⚠️ Usage: task update <task-number> "<task>" ["<category>"]', fg=typer.colors.YELLOW) + return + if len(parts) < 2 or not parts[0].isdigit(): typer.secho('⚠️ Usage: task update <task-number> "<task>" ["<category>"]', fg=typer.colors.YELLOW) return index = int(parts[0]) - 1 - task_text = parts[1].strip('"') if len(parts) >= 2 else None - category = parts[2].strip('"') if len(parts) == 3 else None + task_text = parts[1] if len(parts) >= 2 else None + category = parts[2] if len(parts) == 3 else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@trushell/commands/tasks.py` around lines 86 - 101, The current update_task function incorrectly uses args.split(maxsplit=2) which doesn’t respect quotes; replace that logic with shlex.split(args) (like add_task) to properly parse quoted multi-word task text and optional category, then extract index, task_text and category from the resulting parts list before calling update_todo; keep the same validation (ensure parts[0] is a digit and length >=2) and preserve the existing variables index, task_text and category handling and error reporting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@trushell/commands/tasks.py`:
- Around line 86-101: The current update_task function incorrectly uses
args.split(maxsplit=2) which doesn’t respect quotes; replace that logic with
shlex.split(args) (like add_task) to properly parse quoted multi-word task text
and optional category, then extract index, task_text and category from the
resulting parts list before calling update_todo; keep the same validation
(ensure parts[0] is a digit and length >=2) and preserve the existing variables
index, task_text and category handling and error reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d95abcc-95d8-48f4-9522-465335cd22ec
📒 Files selected for processing (2)
tests/test_help_docs.pytrushell/commands/tasks.py
💤 Files with no reviewable changes (1)
- tests/test_help_docs.py
|
@zza-830 Please remove the dead |
2 participants
Problem
The
add_task()function hardcodedcategory='General'regardless of what the user passed. Additionally,_handle_todo_command()incli.pyparsed both arguments from the regex match but concatenated them into a single string, discarding the category.Closes #46
Fix
tasks.py::add_task(): Useshlex.split()to properly parse quoted arguments, supporting bothaddtask "task" "category"andaddtask task(defaults to General)cli.py::_handle_todo_command(): Pass arguments with quotes preserved soadd_taskcan parse them correctlyTesting
Summary by CodeRabbit
New Features
Improvements
Tests