Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR represents a major v2.0.0 rewrite of the Toggl Target project, modernizing it from Python 2.7 to Python 3.8+ with contemporary development practices and tooling.
Key Changes
- Complete modernization to Python 3.8+ with type hints, Pydantic models, and modern dependency management (UV/pip)
- New rich CLI interface with beautiful terminal output, multiple output formats (table/JSON/YAML), and improved user experience
- Comprehensive testing infrastructure with pytest, fixtures, and test coverage for core models and configurations
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| toggl-target.yaml.example | New YAML configuration template with API and working time settings |
| pyproject.toml | Modern build configuration using hatchling with dependencies, dev tools, and project metadata |
| .pre-commit-config.yaml | Pre-commit hooks for code quality (black, ruff, mypy) |
| .gitignore | Expanded ignore patterns for modern Python development |
| src/toggl_target/core/models.py | Core data models using Pydantic for validation (Target, WorkingTimeConfig, APIConfig, TimeEntry, Weekday) |
| src/toggl_target/core/calculator.py | Business logic for calculating targets and day counts |
| src/toggl_target/core/api.py | Modern Toggl API client with comprehensive error handling |
| src/toggl_target/core/init.py | Core module exports |
| src/toggl_target/utils/terminal.py | Terminal utilities for progress bars, colors, and formatting |
| src/toggl_target/utils/date_utils.py | Date manipulation utilities for month/day calculations |
| src/toggl_target/utils/init.py | Utility module exports |
| src/toggl_target/config/settings.py | Configuration management supporting YAML files and environment variables |
| src/toggl_target/config/init.py | Config module exports |
| src/toggl_target/cli/main.py | Click-based CLI with commands for status, report, init, and version |
| src/toggl_target/cli/formatters.py | Output formatters for status and reports in multiple formats |
| src/toggl_target/cli/init.py | CLI module exports |
| src/toggl_target/init.py | Package initialization with version 2.0.0 |
| tests/test_models.py | Comprehensive unit tests for data models |
| tests/conftest.py | Pytest fixtures for testing |
| run.py | Modified with problematic duplicate code blocks and debugging comments |
| backup.py | Commented out legacy config import |
| README.md | Completely rewritten documentation with installation, usage, and migration guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def month_end(self) -> datetime: | ||
| """End of the current month.""" | ||
| return self.now.replace(day=1) + relativedelta(months=1, days=-1, hour=23, minute=59, second=59, microsecond=0) |
There was a problem hiding this comment.
The month_end calculation may have an off-by-one error. Using relativedelta with days=-1 after adding a month could give unexpected results depending on the current date. The more common and reliable pattern would be to use relativedelta(months=1, seconds=-1) or simply go to the next month's first day and subtract one day using timedelta. The current approach may not correctly handle edge cases around month boundaries.
| days_str = days_env.upper() | ||
| day_mapping = { | ||
| 'MON': Weekday.MONDAY, | ||
| 'TUE': Weekday.TUESDAY, |
There was a problem hiding this comment.
There is trailing whitespace after "TUE" on line 113. This is inconsistent with the formatting of other lines and should be removed.
| 'TUE': Weekday.TUESDAY, | |
| 'TUE': Weekday.TUESDAY, |
| for day_name in days_str.split(','): | ||
| day_name = day_name.strip() | ||
| if day_name in day_mapping: | ||
| days.append(day_mapping[day_name]) |
There was a problem hiding this comment.
The environment variable parsing for business days silently ignores invalid day names. If a user mistypes a day name (e.g., "MONDAY" instead of "MON"), it will be silently dropped from the list. This could lead to configuration errors that are hard to debug. Consider logging a warning when an invalid day name is encountered, similar to how invalid numeric values are handled for hours and tolerance.
| days.append(day_mapping[day_name]) | |
| days.append(day_mapping[day_name]) | |
| elif day_name: | |
| logger.warning( | |
| "Invalid BUSINESS_DAYS value '%s'. Expected one or more of: %s", | |
| day_name, | |
| ", ".join(sorted(day_mapping.keys())), | |
| ) |
| # Get detailed report in JSON format | ||
| toggl-target report --format json | ||
|
|
||
| # Get detailed report in YAML format |
There was a problem hiding this comment.
There is trailing whitespace after "YAML" on line 73. This should be removed for consistency.
| # Get detailed report in YAML format | |
| # Get detailed report in YAML format |
|
|
||
| Required working hours for this month : 170 | ||
| 1. `toggl-target.yaml` (current directory) | ||
| 2. `toggl-target.yml` (current directory) |
There was a problem hiding this comment.
There is trailing whitespace after "yml" on line 108. This should be removed for consistency.
| 2. `toggl-target.yml` (current directory) | |
| 2. `toggl-target.yml` (current directory) |
|
|
||
| import json | ||
| from datetime import datetime | ||
| from typing import Optional |
There was a problem hiding this comment.
Import of 'Optional' is not used.
| from typing import Optional |
| from rich.console import Console | ||
| from rich.table import Table | ||
| from rich.panel import Panel | ||
| from rich.progress import Progress, BarColumn, TextColumn |
There was a problem hiding this comment.
Import of 'Progress' is not used.
Import of 'BarColumn' is not used.
Import of 'TextColumn' is not used.
| from rich.progress import Progress, BarColumn, TextColumn |
| from toggl_target.utils import ( | ||
| create_progress_bar, | ||
| success_text, | ||
| error_text, | ||
| warning_text, | ||
| info_text, | ||
| format_duration, | ||
| format_number, | ||
| get_month_name, | ||
| get_year_month, | ||
| ) |
There was a problem hiding this comment.
Import of 'error_text' is not used.
|
|
||
| from toggl_target.config import ConfigManager, ConfigError | ||
| from toggl_target.core import TogglAPI, TargetCalculator, TogglAPIError | ||
| from toggl_target.utils import success_text, error_text, warning_text, info_text |
There was a problem hiding this comment.
Import of 'warning_text' is not used.
| from toggl_target.utils import success_text, error_text, warning_text, info_text | |
| from toggl_target.utils import success_text, error_text, info_text |
| """Test data models.""" | ||
|
|
||
| import pytest | ||
| from datetime import datetime |
There was a problem hiding this comment.
Import of 'datetime' is not used.
| from datetime import datetime |
No description provided.