-
-
Notifications
You must be signed in to change notification settings - Fork 10
Typecast refactoring #260
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
Typecast refactoring #260
Conversation
Tigrov
commented
Jul 4, 2023
Q | A |
---|---|
Is bugfix? | ❌ |
New feature? | ❌ |
Breaks BC? | ❌ |
Fixed issues |
PR Summary
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #260 +/- ##
=========================================
Coverage 98.20% 98.20%
+ Complexity 327 323 -4
=========================================
Files 17 17
Lines 1003 1005 +2
=========================================
+ Hits 985 987 +2
Misses 18 18
☔ View full report in Codecov by Sentry. |
return $column; | ||
return match ($defaultValue) { | ||
null, 'null', '' => null, | ||
'CURRENT_TIMESTAMP', 'CURRENT_DATE', 'CURRENT_TIME' => new Expression($defaultValue), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need add check for type of column. Without check type of column - with default value in column equal 'CURRENT_TIMESTAMP' (string), default value will be incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no special date type timestamp
or datetime
in SQlite.
https://www.sqlite.org/datatype3.html
For this purpose text
type can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have dbtype in schema of column, and need check type in condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test with STRICT
table where TIMESTAMP
type is not allowed but TEXT
type for storing date and time is okey.
CREATE TABLE "timestamp_default" (
id INTEGER PRIMARY KEY,
text_col TEXT NOT NULL DEFAULT 'CURRENT_TIMESTAMP',
timestamp_text TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
) STRICT;
Could you show a test where current changes without check for type of column
will fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test will be more reliable. Simply add test for column text_col with assert: defaultValue not instaceof Expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current test is sufficient. Make it work without errors and that's it. Thank you
return $column; | ||
return match ($defaultValue) { | ||
null, 'null', '' => null, | ||
'CURRENT_TIMESTAMP', 'CURRENT_DATE', 'CURRENT_TIME' => new Expression($defaultValue), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have dbtype in schema of column, and need check type in condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Please add line to changelog
Thanks |