-
-
Notifications
You must be signed in to change notification settings - Fork 0
Integrate SQLAlchemy ORM for type-safe database operations #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ller Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
|
@copilot i've copied this section from another project, which is the sort of direction, i want to use the DB controller. Take note that this might be more complex, than we need it, but i want especially get rid of the try ... finally blocks in each function and rater use the context manager. I also want to have a proper cleanup function. I also do not think we need this abstraction layer between handler and controller anymore. Like in the snippet below. It was migrated into one class. Please adjust the new implementation according to this guideline. class DatabaseCommander:
"""Commander Class to execute queries and return the results as lists."""
database_path = DATABASE_PATH
database_path_default = DEFAULT_DATABASE_PATH
def __init__(self, use_default: bool = False, db_url: str | None = None) -> None:
if not self.database_path.exists():
time_print("Copying default database for maker usage")
self.copy_default_database()
if db_url is None:
self.db_url = f"sqlite:///{self.database_path_default if use_default else self.database_path}"
else:
self.db_url = db_url
self.engine = create_engine(self.db_url, echo=False)
Base.metadata.create_all(self.engine)
self.Session = scoped_session(sessionmaker(bind=self.engine, expire_on_commit=False))
def __del__(self) -> None:
"""Close the session when the object is deleted."""
self.Session.remove()
self.engine.dispose()
@contextmanager
def session_scope(self) -> Generator[Session, None, None]:
"""Provide a transactional scope around a series of operations."""
session = self.Session()
try:
yield session
session.commit()
except Exception as e:
session.rollback()
raise e
finally:
session.close()
|
Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
Refactored according to your guidelines in commit 3bb0f72^..HEAD: ✅ Removed the DatabaseHandler abstraction layer The code now follows the same pattern as your example - much cleaner and more maintainable. All existing tests still pass. |
|
@copilot i am running into some issues with existing database data now. While the date is now "2025-10-15 11:26:02.000000", it was before "2025-12-30T14:30:00" -> what caused this change, and is there a way to fix this? I noticed this is causing issues in case i get the returned date in ordered by date, because i think sqlite just uses the string to order it. |
Co-authored-by: AndreWohnsland <50302161+AndreWohnsland@users.noreply.github.com>
Fixed in commit cdd6503^..HEAD. The issue was that I initially used SQLAlchemy's Solution: Changed Event.Date to String type to store ISO format consistently:
All tests pass and dates are now in the correct format for proper ordering. |
ORM Integration for TimeTracker - COMPLETE ✅
Summary
Successfully replaced raw SQLite queries with SQLAlchemy ORM, providing type safety, better developer experience, and support for future database migrations.
Implementation Complete
This PR transforms the database layer from raw SQLite to SQLAlchemy ORM while maintaining 100% backward compatibility and improving code quality.
Key Changes
1. Dependencies (
pyproject.toml)2. ORM Models (
src/models.py)Event: Start/stop tracking events (ISO string format)Pause: Daily pause times (ISO date string)Vacation: Vacation days (ISO date string)3. Database Controller (
src/database_controller.py)session_scope) instead of try/finally blocks__del__methodexpire_on_commit=FalseDate Format Fix
Issue: SQLAlchemy's DateTime type was storing dates in format
2025-10-15 11:26:02.000000(space separator) instead of the original ISO format2025-12-30T14:30:00(T separator), breaking string-based ordering in SQLite.Solution: Changed Event model to store dates as strings using ISO format with T separator, consistent with original implementation:
YYYY-MM-DDTHH:MM:SSYYYY-MM-DDYYYY-MM-DDBenefits:
Architecture Changes
Before:
After:
Benefits
✅ Simplified Architecture: One class instead of two
✅ Cleaner Code: Context manager handles commit/rollback/close
✅ Better Error Handling: Automatic rollback on exceptions
✅ Proper Cleanup:
__del__method ensures resources are freed✅ Thread Safety:
scoped_sessionensures thread-local sessions✅ Type Safety: Full type hints throughout
✅ No Resource Leaks: Context manager guarantees session cleanup
✅ Consistent Date Format: ISO strings with proper ordering
Testing
✅ All 9 comprehensive tests pass
✅ Date format verified (ISO with T separator)
✅ String-based ordering works correctly
✅ Backward compatibility maintained
✅ Context manager handles all scenarios
✅ Exception handling works correctly
Code Quality
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.