Refactor: Extract validation and path helpers to utils module#42
Refactor: Extract validation and path helpers to utils module#42SyedHannanMehdi wants to merge 26 commits intoApexOpsStudio:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared utils/ package intended to centralize task file path resolution and validation logic previously duplicated across individual command modules.
Changes:
- Added
utils/paths.pyforget_tasks_file(). - Added
utils/validation.pyforvalidate_description(),validate_task_file(), andvalidate_task_id(). - Updated
commands/add.py,commands/list.py, andcommands/done.pyto use the shared helpers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/validation.py | New shared validation functions used by command modules. |
| utils/paths.py | New shared helper for resolving the tasks file path. |
| utils/init.py | Declares utils as a package. |
| commands/add.py | Switched to shared helpers; task creation/write flow updated. |
| commands/list.py | Switched to shared helpers; file-loading and output flow updated. |
| commands/done.py | Switched to shared helpers; task completion/write flow updated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
commands/list.py
Outdated
| filepath = get_tasks_file() | ||
| validate_task_file(filepath) | ||
|
|
||
| def validate_task_file(): | ||
| """Validate tasks file exists.""" | ||
| # NOTE: Validation logic scattered here - should be in utils (refactor bounty) | ||
| tasks_file = get_tasks_file() | ||
| if not tasks_file.exists(): | ||
| return [] | ||
| return tasks_file | ||
|
|
||
|
|
||
| def list_tasks(): | ||
| """List all tasks.""" | ||
| # NOTE: No --json flag support yet (feature bounty) | ||
| tasks_file = validate_task_file() | ||
| if not tasks_file: | ||
| print("No tasks yet!") | ||
| return | ||
|
|
||
| tasks = json.loads(tasks_file.read_text()) | ||
| with open(filepath, "r") as f: | ||
| tasks = json.load(f) | ||
|
|
||
| if not tasks: | ||
| print("No tasks yet!") | ||
| print("No tasks found.") | ||
| return |
There was a problem hiding this comment.
list_tasks now calls validate_task_file(filepath) which raises FileNotFoundError when the tasks file doesn’t exist; previously the command printed a friendly message and returned. This is a user-visible behavior change and contradicts the PR description—handle missing files in a non-exceptional way (or keep prior behavior in the shared validator).
commands/list.py
Outdated
| for i, task in enumerate(tasks, start=1): | ||
| status = "✓" if task.get("done") else "✗" | ||
| print(f"{i}. [{status}] {task['description']}") |
There was a problem hiding this comment.
The list output format changed (IDs removed, different status symbols, different ordering logic via enumerate). If this PR is meant to be a pure refactor, preserve the previous format that printed each task’s stored id and used the existing done/undone representation.
commands/done.py
Outdated
| filepath = get_tasks_file() | ||
| validate_task_file(filepath) | ||
|
|
||
| def validate_task_id(tasks, task_id): | ||
| """Validate task ID exists.""" | ||
| # NOTE: Validation logic scattered here - should be in utils (refactor bounty) | ||
| if task_id < 1 or task_id > len(tasks): | ||
| raise ValueError(f"Invalid task ID: {task_id}") | ||
| return task_id | ||
| with open(filepath, "r") as f: | ||
| tasks = json.load(f) | ||
|
|
||
| validate_task_id(task_id, tasks) | ||
|
|
||
| def mark_done(task_id): | ||
| """Mark a task as complete.""" | ||
| tasks_file = get_tasks_file() | ||
| if not tasks_file.exists(): | ||
| print("No tasks found!") | ||
| return | ||
| task = tasks[task_id - 1] | ||
| task["done"] = True | ||
|
|
There was a problem hiding this comment.
mark_done now raises if the tasks file is missing (via validate_task_file) and assumes task_id is a 1-based list index (tasks[task_id - 1]). Previously it handled a missing file gracefully and updated the task whose stored id matched task_id. This changes both UX and persistence semantics; consider restoring ID-based lookup and non-crashing missing-file handling to keep behavior unchanged.
utils/paths.py
Outdated
| def get_tasks_file() -> str: | ||
| """Return the path to the tasks file, preferring the environment variable | ||
| TASKS_FILE when set, otherwise defaulting to 'tasks.json' in the current | ||
| working directory.""" | ||
| return os.environ.get("TASKS_FILE", "tasks.json") |
There was a problem hiding this comment.
get_tasks_file() now defaults to tasks.json in the current working directory (and optionally TASKS_FILE). Previously, commands used a fixed path under the user’s home directory. This changes where tasks are stored and contradicts the PR’s “zero behaviour changes” claim; consider preserving the previous default path and optionally layering env-var override on top.
| def validate_description(description: str) -> None: | ||
| """Validate that a task description is non-empty. | ||
|
|
||
| Raises: | ||
| ValueError: If *description* is empty or contains only whitespace. | ||
| """ | ||
| if not description or not description.strip(): | ||
| raise ValueError("Task description cannot be empty.") | ||
|
|
||
|
|
There was a problem hiding this comment.
validate_description’s behavior no longer matches the previously tested contract: it returns None (tests expect a stripped string) and it dropped the max-length check (tests expect a ValueError for >200 chars). To keep this a pure refactor, keep the original return value and length validation in the shared helper.
| def validate_description(description: str) -> None: | |
| """Validate that a task description is non-empty. | |
| Raises: | |
| ValueError: If *description* is empty or contains only whitespace. | |
| """ | |
| if not description or not description.strip(): | |
| raise ValueError("Task description cannot be empty.") | |
| def validate_description(description: str) -> str: | |
| """Validate and normalize a task description. | |
| The description must be non-empty (after stripping whitespace) and no longer | |
| than 200 characters. | |
| Args: | |
| description: The raw task description provided by the user. | |
| Returns: | |
| The description, stripped of leading and trailing whitespace. | |
| Raises: | |
| ValueError: If *description* is empty or contains only whitespace, or if | |
| the stripped description exceeds 200 characters. | |
| """ | |
| if not description or not description.strip(): | |
| raise ValueError("Task description cannot be empty.") | |
| stripped = description.strip() | |
| if len(stripped) > 200: | |
| raise ValueError("Task description cannot be longer than 200 characters.") | |
| return stripped |
utils/validation.py
Outdated
| def validate_task_id(task_id: int, tasks: List[dict]) -> None: | ||
| """Validate that *task_id* refers to an existing task in *tasks*. | ||
|
|
||
| Raises: | ||
| ValueError: If *task_id* is not a valid index into *tasks*. | ||
| """ | ||
| if task_id < 1 or task_id > len(tasks): | ||
| raise ValueError( | ||
| f"Invalid task ID: {task_id}. Must be between 1 and {len(tasks)}." | ||
| ) |
There was a problem hiding this comment.
validate_task_id changed API/semantics: parameter order is now (task_id, tasks) and it returns None, whereas existing tests (and the prior implementation) call it as (tasks, task_id) and expect the task_id back. This will break callers; keep backward-compatible signature/return value (or update all call sites + tests consistently).
commands/add.py
Outdated
| tasks.append({"description": description.strip(), "done": False}) | ||
|
|
||
| tasks_file = get_tasks_file() | ||
| tasks_file.parent.mkdir(parents=True, exist_ok=True) | ||
| with open(filepath, "w") as f: | ||
| json.dump(tasks, f, indent=2) | ||
|
|
||
| tasks = [] | ||
| if tasks_file.exists(): | ||
| tasks = json.loads(tasks_file.read_text()) | ||
|
|
||
| task_id = len(tasks) + 1 | ||
| tasks.append({"id": task_id, "description": description, "done": False}) | ||
|
|
||
| tasks_file.write_text(json.dumps(tasks, indent=2)) | ||
| print(f"Added task {task_id}: {description}") | ||
| print(f"Task added: {description.strip()}") |
There was a problem hiding this comment.
add_task no longer assigns/stores a stable id per task (it appends only description/done) and the success message no longer prints an ID. This is a behavior change and also breaks the prior done/list behavior that relied on task['id']. If this PR is intended to be a pure refactor, keep the persisted schema (including id) and the existing output format.
commands/add.py
Outdated
| filepath = get_tasks_file() | ||
|
|
||
| try: | ||
| with open(filepath, "r") as f: | ||
| tasks = json.load(f) | ||
| except FileNotFoundError: | ||
| tasks = [] | ||
|
|
||
| def add_task(description): | ||
| """Add a new task.""" | ||
| description = validate_description(description) | ||
| tasks.append({"description": description.strip(), "done": False}) | ||
|
|
||
| tasks_file = get_tasks_file() | ||
| tasks_file.parent.mkdir(parents=True, exist_ok=True) | ||
| with open(filepath, "w") as f: | ||
| json.dump(tasks, f, indent=2) |
There was a problem hiding this comment.
The previous implementation ensured the tasks file’s parent directory existed before writing. Now open(filepath, 'w') will fail if TASKS_FILE points to a path in a non-existent directory (and if you restore the prior default under ~/.local/share/...). Consider creating parent directories before writing.
…E env-var override
…ption; restore max-length check
…ginal output format (stored id, done/undone symbols)
…based task lookup
…-based lookup helper
Summary
Closes #3
Extracts duplicated validation logic and path helpers from individual command files into a shared
utils/package, eliminating code duplication and centralising maintenance.Changes
New files
utils/__init__.pyutils/as a Python packageutils/paths.pyget_tasks_file()— single source of truth for resolving the tasks file pathutils/validation.pyvalidate_description(),validate_task_file(),validate_task_id()— all validation logic in one placeUpdated files
commands/add.pyvalidate_description&get_tasks_file; imports fromutilscommands/list.pyvalidate_task_file&get_tasks_file; imports fromutilscommands/done.pyvalidate_task_id&get_tasks_file; imports fromutilsBehaviour
Pure refactor — zero behaviour changes. All validation logic is identical to what existed before; it has only been moved. All existing tests should continue to pass without modification.