Replace 'Maybe Bool' with a dedicated sum-type. #1717
Conversation
KtorZ
added
the
RESOLVING ISSUE
KtorZ
force-pushed
the
KtorZ/1701/passphrase-scheme-fix
branch
from
7c3d459 to
0c24254
Compare
| = TableDoesntExist | ||
| | ColumnDoesntExist | ||
| | ColumnExists |
There was a problem hiding this comment.
Perhaps this wording would be a little more concise:
| = TableDoesntExist | |
| | ColumnDoesntExist | |
| | ColumnExists | |
| = TableMissing | |
| | ColumnMissing | |
| | ColumnPresent |
| @@ -306,6 +306,13 @@ findDatabases tr dir = do | |||
| where | |||
| expectedPrefix = T.pack $ keyTypeDescriptor $ Proxy @k | |||
|
|
|||
| -- | A data-type for capturing column status. Used to be represented as a | |||
| -- 'Maybe Bool' which is somewhat confusing to interpret. | |||
| data SQLColumnStatus | |||
There was a problem hiding this comment.
Suggestion: use PascalCase here, treating SQL as a word.
| data SQLColumnStatus | |
| data SqlColumnStatus |
Similar examples:
There was a problem hiding this comment.
I do actually prefer the golang guidelines on acronyms, but whatever ^.^
KtorZ
force-pushed
the
KtorZ/1701/passphrase-scheme-fix
branch
from
0c24254 to
d96d0a0
Compare
| -- | A data-type for capturing column status. Used to be represented as a | ||
| -- 'Maybe Bool' which is somewhat confusing to interpret. |
There was a problem hiding this comment.
Suggestion: remove the line starting with "Used to be represented...".
Justification: the new code is already very clear. 👍
(We don't need to remind the reader how confusing it used to be 😸)
| -- | A data-type for capturing column status. Used to be represented as a | |
| -- 'Maybe Bool' which is somewhat confusing to interpret. | |
| -- | Indicates whether or not a column and its parent table are present | |
| -- in the database. |
|
Unfortunately I still see behavior as in: #1552 To reproduce:
|
|
What is interesting that after point 4 (from the above) when you restart wallet again (using the same version) you are able to send transaction... 🤔 |
|
@piotr-iohk My guess here is that, when you first restart it, the automatic migration hasn't happened yet, so the column doesn't yet exists and can't be populated (so, running into the bug as before). Manual migrations are ran BEFORE automatic migration. which is, somewhat necessary for the active slot coeff as it requires to alter the tables before attempting an automatic migration, but problematic for this one which requires the passphrase_scheme to be set :| ... We could "force-create" the column if it doesn't exist. It won't hurt and we are sure it's there. |
Unfortunately, the manual migration are applied before the automated migration. Yet, the passphraseScheme migration requires the column to already exists in order to populate it. Therefore, we make sure to first create it, and then, can safely assign it a value. If it already exists, we simply assign a value.
|
bors r+ |
Build succeeded |
Issue Number
#1701
Overview
9aaf4ad
📍 log when no migration is needed
7c3d459
📍 replace 'Maybe Bool' with a dedicated sum-type for more clarity
Comments
Caught me one time already... Won't caught anyone else.