refactor: almost final migrations cleanup by Computerdores · Pull Request #1456 · TagStudioDev/TagStudio

Computerdores · 2026-07-23T19:54:41Z

Summary

Progress on #1442.
Aims to complete the migrations cleanup far enough that only independence from sqlalchemy remains.

Tasks Completed

Platforms Tested:
- Windows x86
- Windows ARM
- macOS x86
- macOS ARM
- Linux x86
- Linux ARM
Tested For:
- Basic functionality
- PyInstaller executable

Bumping the auto increment value has been done since the original sql PR, so it doesn't need to be done on migrations. See e5e7b8a.

The only table that has been added since DB version 6 (the earliest supported version), is the versions table in DB version 101. This commit removes the "create all tables" statement, and instead creates the versions table in the 101 migration. See 12e074b.

CyanVoxel

Looks like somethings not quite right with the migrations, here I threw in a DB 9 library and it failed on migration 200:

2026-07-24 03:51:02 [info     ] [Library][Migration][200] Starting DB Migration
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Dropping boolean_fields and value_type tables...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Adding name columns to field tables...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Dropping position columns to field tables...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Adding is_multiline column to text_fields...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Moving values from type_key columns to name...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Normalizing TextField names...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Normalizing DatetimeField names...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Updating is_multiline for legacy TEXT_BOXes...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Repairing legacy Description fields...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Repairing legacy Comment fields...
2026-07-24 03:51:02 [info     ] [Library][Migration][200] Adding default field templates...
2026-07-24 03:51:02 [error    ] (sqlite3.OperationalError) no such table: datetime_field_templates
[SQL: INSERT INTO datetime_field_templates (name) VALUES (?)]
[parameters: ('Date',)]
(Background on this error at: https://sqlalche.me/e/20/e3q8)

Verified working on main

CyanVoxel · 2026-07-24T10:37:02Z

+                            "Couldn't update version, continuing without commit",
+                            error=e,
+                        )
+                        session.flush()


Why the session.flush() here?

A fallback in case the migration doesn't flush on its own

CyanVoxel · 2026-07-24T10:44:34Z

@@ -2160,7 +1778,8 @@ def get_version(self, key: str) -> int:
            except Exception:
                return 0

-    def set_version(self, session: Session, key: str, value: int) -> None:
+    @staticmethod
+    def _set_version(session: Session, key: str, value: int) -> None:


These should be public if they're used in DBMigrations

I have moved _set_version to DBMigrations.__set_version as it isn't used anywhere else.

I still think it would be useful to differentiate between _ and __, where _ essentially means "private, except for a small number of exceptions". Because in this case for example I intended _get_version to only be used in the .alchemy submodule as it directly touches sql stuff (also there would be a name collision with get_version if the _ were removed). Other examples are for example when a private function needs to be used by the tests, where it doesn't make sense to make it public, but keeping it private (via __) isn't possible either.
Another advantage would that looking at a function with __ immediately tells you that it isn't used outside the class, whereas _ leaves that open.

The reason I ended up souring on the _ and __ delineation is because in practice it became very difficult to keep up with, where I found myself constantly refactoring variable names to add or remove underscores across more and more affected files while simultaneously getting frustrated at having to type "Shift+-+-" over and over again just to start typing variable names - all for what is a superficial meaning placed onto variables for a language that doesn't actually enforce these concepts at best, and implements them differently in a way we're not respecting (i.e. name mangling) at worst.

It sounds like a good idea on paper, and believe me if Python had actual private and protected keywords I'd be all over them, but in practice the two types of prefixes is just more of a burden than they're worth imo, and a single underscore will do just fine to indicate intended private usage. If an attribute is meant to be used sometimes publicly (tests being an exception), then it should just be public - and if there are aspects of an attribute that shouldn't be allowed publicly, then those should be hidden away somehow, like a read-only getter property.

Computerdores · 2026-07-24T14:17:53Z

Looks like somethings not quite right with the migrations, here I threw in a DB 9 library and it failed on migration 200:

Yeah turns out the one time I didn't test, I overlooked something ^^'

I forgot that adding relationships also adds tables that need to be created.
Fyi I'll get around to that, but I don't have time today, I'll let you know when that's done

CyanVoxel · 2026-07-24T18:56:52Z

+
+
+class DBMigration:
+    version: int = None  # pyright: ignore[reportAssignmentType]


This (should) be able to just be unbound to avoid the pyright error

Suggested change

version: int = None # pyright: ignore[reportAssignmentType]

version: int

purpletennisball · 2026-07-25T07:43:59Z

+                        session.flush()
+                    else:
+                        session.commit()
+                logger.info(f"[Library][Migration][{migration.version}] Completed DB Migration")


Wouldn't this print even if the migration fails? Just a little nit pick

Computerdores added 7 commits July 23, 2026 19:20

fix: backup library before making any changes

617ff71

refactor: inline make_tables + minor cleanup

0d1597d

refactor: remove unnecessary assurance

24f9f27

Bumping the auto increment value has been done since the original sql PR, so it doesn't need to be done on migrations. See e5e7b8a.

refactor: don't require setting library_dir to create a backup

a22482d

refactor: move migrations to different file

92809cc

refactor: package each migration in a class

eef0da6

Computerdores requested a review from CyanVoxel July 23, 2026 19:54

Computerdores added this to TagStudio Development Jul 23, 2026

Computerdores added Type: Refactor Code that needs to be restructured or cleaned up TagStudio: Library Relating to the TagStudio library system labels Jul 23, 2026

Computerdores marked this pull request as draft July 23, 2026 19:56

Computerdores removed the request for review from CyanVoxel July 23, 2026 19:56

Computerdores added 2 commits July 23, 2026 22:09

fix: some syntax errors had slipped through

75c586d

fix: allow set_version to fail, but don't commit in that case

200a285

Computerdores moved this to 🚧 In progress in TagStudio Development Jul 23, 2026

Computerdores moved this from 🚧 In progress to 🏓 Ready for Review in TagStudio Development Jul 23, 2026

Computerdores marked this pull request as ready for review July 23, 2026 21:33

Computerdores requested a review from CyanVoxel July 23, 2026 21:34

CyanVoxel moved this from 🏓 Ready for Review to 👀 In review in TagStudio Development Jul 24, 2026

CyanVoxel reviewed Jul 24, 2026

View reviewed changes

CyanVoxel added the Priority: Medium An issue that shouldn't be be saved for last label Jul 24, 2026

Computerdores added 3 commits July 24, 2026 16:00

refactor: condense imports

c799450

fix: add override decorators

ccd07ee

refactor: move set_version to DBMigrations

92b40a7

CyanVoxel reviewed Jul 24, 2026

View reviewed changes

purpletennisball reviewed Jul 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor: almost final migrations cleanup#1456

refactor: almost final migrations cleanup#1456
Computerdores wants to merge 12 commits into
mainfrom
refactor-almost-final-migrations-cleanup

Computerdores commented Jul 23, 2026

Uh oh!

CyanVoxel left a comment

Uh oh!

Uh oh!

CyanVoxel Jul 24, 2026

Uh oh!

Computerdores Jul 24, 2026

Uh oh!

Uh oh!

CyanVoxel Jul 24, 2026

Uh oh!

Computerdores Jul 24, 2026 •

edited

Loading

Uh oh!

CyanVoxel Jul 24, 2026

Uh oh!

Uh oh!

Computerdores commented Jul 24, 2026

Uh oh!

CyanVoxel Jul 24, 2026

Uh oh!

purpletennisball Jul 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants



		class DBMigration:
		version: int = None # pyright: ignore[reportAssignmentType]

	version: int = None # pyright: ignore[reportAssignmentType]
	version: int

Uh oh!

Uh oh!

Conversation

Computerdores commented Jul 23, 2026

Summary

Tasks Completed

Uh oh!

CyanVoxel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyanVoxel Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Computerdores Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyanVoxel Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Computerdores Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyanVoxel Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Computerdores commented Jul 24, 2026

Uh oh!

CyanVoxel Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

purpletennisball Jul 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Computerdores Jul 24, 2026 •

edited

Loading