Unsubscribe: Tables altered and new functions added #53
Conversation
|
|
||
|
|
||
| def upgrade() -> None: | ||
| op.add_column("accounts", sa.Column("is_deleted", sa.Boolean, nullable=True)) |
There was a problem hiding this comment.
Do we want nullable here? I recognize that you need to make it nullable to add it, but do we want the "final form" of this column to be nullable, or do we want it default-false? What's the difference between is_deleted = null and is_deleted=False?
(To do this if memory serves you do an add column that's nullable, update all rows to false, then modify to be non-null)
There was a problem hiding this comment.
I had the same approach as well, but went the other way. I shall make the changes
There was a problem hiding this comment.
That's not the wrong thing to do, but I suggested skipping it because of a defaults-related hassle:
SQLAlchemy treats non-nullable fields with database-level defaults as required, even though omitting them should theoretically be allowed. If this PR makes the new field non-nullable, it will also need to update the repository code to make sure that field is set whenever we store accounts.
| """ | ||
| from typing import Sequence, Union | ||
|
|
||
| from alembic import op |
There was a problem hiding this comment.
we can probably get this fixed when we want to merge. Manual fixing these isn't too hard, but I think pre-commit run --all-files or something similar should also fix the problem.
|
@aisa6148 I think this just needs a few unused imports to be removed from the database migrations in order to pass the CI checks. Otherwise this looks ready to merge? |
|
@karlhigley All checks have passed for this one (although the error sent over slack persists). Apart from that this branch is updated with main and is ready to merge! |
(Need to test after merging main)
Unsubscribe from emails