rogs/cleanmedia
rogs/cleanmedia
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

feat: add support for sqlite3 and multiple databases #2

Open
cgroschupp wants to merge 8 commits from feature/sqlite3-and-multiple-db-support into master
pull from: feature/sqlite3-and-multiple-db-support
merge into: rogs:master
Conversation 27 Commits 8 Files Changed 2 +164 -113
cgroschupp commented 2025-03-27 07:28:55 -03:00 (Migrated from gitlab.com)
Copy Link
No description provided.
cgroschupp commented 2025-03-27 07:31:30 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • 8bd50ed6 - ci: use uv in pipeline

Compare with previous version

added 1 commit <ul><li>8bd50ed6 - ci: use uv in pipeline</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1308920701&start_sha=94b62fe7e5a64aa39f9ced27ea4d3caf2c9d4197)
cgroschupp commented 2025-03-27 07:44:05 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • 357aafdf - ci: use uv in pipeline

Compare with previous version

added 1 commit <ul><li>357aafdf - ci: use uv in pipeline</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1308941374&start_sha=8bd50ed615396732b7b030b54ef4c7e024d315f3)
cgroschupp commented 2025-03-27 07:46:08 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • d43669f5 - ci: use uv in pipeline

Compare with previous version

added 1 commit <ul><li>d43669f5 - ci: use uv in pipeline</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1308944670&start_sha=357aafdfac2cf25c544030cea1319c4d930c411d)
cgroschupp commented 2025-03-27 07:49:43 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • 93dbcb19 - style: lint code

Compare with previous version

added 1 commit <ul><li>93dbcb19 - style: lint code</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1308950600&start_sha=d43669f51473134c5740167431263157167fbb78)
cgroschupp commented 2025-03-27 07:51:31 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • 6c01a927 - style: lint code

Compare with previous version

added 1 commit <ul><li>6c01a927 - style: lint code</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1308953671&start_sha=93dbcb19f51f6d76e7e2396aa835eeb3548c92e1)
cgroschupp commented 2025-03-27 08:11:56 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • ecca8d98 - fix: ignore some type checks in test file

Compare with previous version

added 1 commit <ul><li>ecca8d98 - fix: ignore some type checks in test file</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1308988566&start_sha=6c01a927c77e47ee66d444fefd80bc5d8a00a664)
cgroschupp commented 2025-03-27 08:12:40 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • 35e54b20 - fix: ignore some type checks in test file

Compare with previous version

added 1 commit <ul><li>35e54b20 - fix: ignore some type checks in test file</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1308989825&start_sha=ecca8d987a8fdb4b4669f9dd41f788339fcad55a)
cgroschupp commented 2025-03-27 08:20:31 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • bf4d55de - fix: change sql format

Compare with previous version

added 1 commit <ul><li>bf4d55de - fix: change sql format</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1309002898&start_sha=35e54b205df1478612f2c392ee87b142527c6eed)
rogs commented 2025-03-27 08:31:41 -03:00 (Migrated from gitlab.com)
Copy Link

First of all, thank you for your work!

Although I do agree that moving to uv makes sense, I would have liked to see 2 PRs for this. It doesn't matter now, it works!

Codewise it looks fine, but I'll do some personal testing later today 👍

Again, thank you!

First of all, thank you for your work! Although I do agree that moving to `uv` makes sense, I would have liked to see 2 PRs for this. It doesn't matter now, it works! Codewise it looks fine, but I'll do some personal testing later today :thumbsup: Again, thank you!
rogs commented 2025-03-27 08:31:44 -03:00 (Migrated from gitlab.com)
Copy Link

requested review from @rogs

requested review from @rogs
cgroschupp commented 2025-03-27 08:37:06 -03:00 (Migrated from gitlab.com)
Copy Link

mentioned in merge request !2

mentioned in merge request !2
cgroschupp commented 2025-03-27 08:38:24 -03:00 (Migrated from gitlab.com)
Copy Link

@rogs i created https://gitlab.com/rogs/cleanmedia/-/merge_requests/2, please merge it before, than i rebase this pr.

@rogs i created https://gitlab.com/rogs/cleanmedia/-/merge_requests/2, please merge it before, than i rebase this pr.
rogs commented 2025-03-27 08:39:17 -03:00 (Migrated from gitlab.com)
Copy Link

I was just reading the PR! Thank you for your work! 🔥

I was just reading the PR! Thank you for your work! :fire:
cgroschupp commented 2025-03-27 08:48:03 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • e72dea95 - docs: update doc strings

Compare with previous version

added 1 commit <ul><li>e72dea95 - docs: update doc strings</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1309046338&start_sha=bf4d55de2a8b413f33cf8cc579c39ef140c87aac)
cgroschupp commented 2025-03-27 08:48:29 -03:00 (Migrated from gitlab.com)
Copy Link

added 1 commit

  • 20add79f - docs: update doc strings

Compare with previous version

added 1 commit <ul><li>20add79f - docs: update doc strings</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1309046982&start_sha=e72dea95a9484c001de0f496b9d34f860d2ca954)
rogs commented 2025-03-27 20:15:55 -03:00 (Migrated from gitlab.com)
Copy Link

Sorry I merged !2 without checking the dockerfile and things broke. If you are moving the project to uv, you'll need to also fix the dockerfile

Sorry I merged !2 without checking the dockerfile and things broke. If you are moving the project to `uv`, you'll need to also fix the dockerfile
cgroschupp commented 2025-03-28 18:48:42 -03:00 (Migrated from gitlab.com)
Copy Link

added 14 commits

  • 20add79f...f80b805a - 6 commits from branch rogs:master
  • d318296e - feat: support sqlite3 and multiple databases
  • 9751bf28 - feat: calculate saved space
  • 19476c59 - feat: update tests
  • 5b248cd4 - style: lint code
  • fdf878d2 - fix: ignore some type checks in test file
  • 78fb13fb - fix: change sql format
  • 016d7db6 - docs: update doc strings
  • 3efc1a9d - style: sort imports

Compare with previous version

added 14 commits <ul><li>20add79f...f80b805a - 6 commits from branch <code>rogs:master</code></li><li>d318296e - feat: support sqlite3 and multiple databases</li><li>9751bf28 - feat: calculate saved space</li><li>19476c59 - feat: update tests</li><li>5b248cd4 - style: lint code</li><li>fdf878d2 - fix: ignore some type checks in test file</li><li>78fb13fb - fix: change sql format</li><li>016d7db6 - docs: update doc strings</li><li>3efc1a9d - style: sort imports</li></ul> [Compare with previous version](/rogs/cleanmedia/-/merge_requests/1/diffs?diff_id=1311157468&start_sha=20add79f747ad018984dbecf55a48e010afc0337)
cgroschupp commented 2025-04-01 17:06:14 -03:00 (Migrated from gitlab.com)
Copy Link

@rogs Is anything still open?

@rogs Is anything still open?
rogs commented 2025-04-01 17:20:16 -03:00 (Migrated from gitlab.com)
Copy Link

Oh, I was waiting for your rebase! I'll check this out now 👍

How did you test this locally?

Oh, I was waiting for your rebase! I'll check this out now :thumbsup: How did you test this locally?
cgroschupp commented 2025-04-01 17:23:54 -03:00 (Migrated from gitlab.com)
Copy Link

See here https://gitlab.com/rogs/cleanmedia/-/merge_requests/3#note_2422397019

See here https://gitlab.com/rogs/cleanmedia/-/merge_requests/3#note_2422397019
rogs commented 2025-04-01 17:24:03 -03:00 (Migrated from gitlab.com)
Copy Link

This should be "Invalid DB connection string"

        raise ValueError("Invalid DB connection string")
This should be "Invalid DB connection string" ```suggestion:-0+0 raise ValueError("Invalid DB connection string") ```
rogs commented 2025-04-01 17:24:03 -03:00 (Migrated from gitlab.com)
Copy Link

A quick overhead review for now, will dig deeply soon

A quick overhead review for now, will dig deeply soon
rogs commented 2025-04-01 17:24:03 -03:00 (Migrated from gitlab.com)
Copy Link

requested changes

requested changes
rogs commented 2025-04-01 17:28:07 -03:00 (Migrated from gitlab.com)
Copy Link

I thought those tests were using the code from master, since the goal was to only test the uv change (that was the purpose of that MR). If you tested that other MR with this code, then the previous test you did for that MR doesn’t really count.

It worked, but we should be more careful going forward.

I thought those tests were using the code from `master`, since the goal was to only test the `uv` change (that was the purpose of that MR). If you tested that other MR with this code, then the previous test you did for that MR doesn’t really count. It worked, but we should be more careful going forward.
rogs commented 2025-04-01 17:36:43 -03:00 (Migrated from gitlab.com)
Copy Link

In the _execute method, consider this modification to ensure cursor cleanup:

def _execute(self, query: str, params: Params = (), db_type: str = "media_api") -> DBCursor:
    paramstyle = getattr(self.conn[db_type], "paramstyle", "format")
    query = self._adjust_paramstyle(query, paramstyle)
    
    # Create a cursor with context management
    class ManagedCursor:
        def __init__(self, conn, query, params):
            self.conn = conn
            self.cursor = None
            self.query = query
            self.params = params
            
        def __enter__(self):
            self.cursor = self.conn.cursor()
            self.cursor.execute(self.query, self.params)
            return self.cursor
            
        def __exit__(self, exc_type, exc_val, exc_tb):
            if self.cursor:
                self.cursor.close()
    
    return ManagedCursor(self.conn[db_type], query, params)

This would allow callers to use with statements again:

with self._execute(query, params) as cursor:
    row = cursor.fetchone()
    # No need to worry about cursor cleanup
In the `_execute` method, consider this modification to ensure cursor cleanup: ```python def _execute(self, query: str, params: Params = (), db_type: str = "media_api") -> DBCursor: paramstyle = getattr(self.conn[db_type], "paramstyle", "format") query = self._adjust_paramstyle(query, paramstyle) # Create a cursor with context management class ManagedCursor: def __init__(self, conn, query, params): self.conn = conn self.cursor = None self.query = query self.params = params def __enter__(self): self.cursor = self.conn.cursor() self.cursor.execute(self.query, self.params) return self.cursor def __exit__(self, exc_type, exc_val, exc_tb): if self.cursor: self.cursor.close() return ManagedCursor(self.conn[db_type], query, params) ``` This would allow callers to use with statements again: ``` with self._execute(query, params) as cursor: row = cursor.fetchone() # No need to worry about cursor cleanup ```
rogs commented 2025-04-01 17:36:43 -03:00 (Migrated from gitlab.com)
Copy Link

I'm concerned about the removal of the context managers (with statements) for database cursors. The new _execute method creates cursors but doesn't ensure they're properly closed after use except in error cases.

This could lead to resource leaks, especially since callers of this method don't appear to close the cursors either. For example, in get_single_media() and similar methods, there's no cursor closing after fetching results.

Could we modify the implementation to either:

  1. Return a context manager that automatically closes cursors
  2. Return to using with statements for operations that need a cursor
I'm concerned about the removal of the context managers (`with` statements) for database cursors. The new `_execute` method creates cursors but doesn't ensure they're properly closed after use except in error cases. This could lead to resource leaks, especially since callers of this method don't appear to close the cursors either. For example, in `get_single_media()` and similar methods, there's no cursor closing after fetching results. Could we modify the implementation to either: 1. Return a context manager that automatically closes cursors 2. Return to using `with` statements for operations that need a cursor
rogs commented 2025-04-01 17:36:43 -03:00 (Migrated from gitlab.com)
Copy Link

requested changes

requested changes
This pull request is broken due to missing fork information.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/sqlite3-and-multiple-db-support:feature/sqlite3-and-multiple-db-support
git checkout feature/sqlite3-and-multiple-db-support
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
rogs
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rogs/cleanmedia#2
No description provided.
Delete Branch "feature/sqlite3-and-multiple-db-support"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?