-
Notifications
You must be signed in to change notification settings - Fork 5
add transcoding between DB and DSQL + Test #8
Conversation
* add transcode table for field type => datatype database * tested on SQLite and MySQL
more space is better than less, this class is very useful during development, after that will be disabled and database must be optimized with other tools. i changed because i had a problem storing serialized EXIF in array datatype
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.
Interesting and looks like very good PR. I like this idea of using proxy transcoding.
But please:
- replace Tab to 4 spaces and Windows line ends \r\n to Linux line ends - just \n
- variable names - we use first lowercase letter most of the time etc.
- code formatting is important for code to look nice and easily readable
- and please try to address at least part of my comments
When that will be done, then I will review one more time more closely and including tests.
But really - good job, thanks!
src/Migration.php
Outdated
'integer' => 'INT4', | ||
'string' => 'VARCHAR256', | ||
'password' => 'VARCHAR256', | ||
'double' => 'DOUBLE', |
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.
no double
type in Agile Data
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.
i know, but in case we need to store more precision ?
src/Migration.php
Outdated
'time' => 'TIME', | ||
'text' => 'TEXT', | ||
'array' => 'TEXT', | ||
'object' => 'BLOB', |
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.
it's serialized as json or base64 - all of them are TEXT not BLOB by nature, so i think using TEXT is better here.
src/Migration.php
Outdated
@@ -77,7 +296,7 @@ public function setModel(\atk4\data\Model $m) | |||
{ | |||
$this->table($m->table); | |||
|
|||
foreach ($m->elements as $field) { | |||
foreach ($m->elements as $field) { |
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.
formatting uses Tabs. Please use 4 spaces instead of tabs everywhere
src/Migration.php
Outdated
/** | ||
* Get Transcode Type Key from Model type | ||
* | ||
* @param field $field DSQL field |
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.
incorrect param type
src/Migration.php
Outdated
return $type; | ||
} | ||
/** | ||
* Get Transcode Type Key from Model type |
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.
incorrect description
climate error correction
switch to codeformatting PSR-1,PSR-2 add hasOne in Test
…" DB fixed mysql float was mistyped uppercase
Codecov Report
@@ Coverage Diff @@
## develop #8 +/- ##
=============================================
- Coverage 72.33% 65.98% -6.35%
- Complexity 129 146 +17
=============================================
Files 5 5
Lines 300 344 +44
=============================================
+ Hits 217 227 +10
- Misses 83 117 +34
Continue to review full report at Codecov.
|
i dunno if this is the direction of this library, but i add some transcode to normalize different PDO drivers and DSQL, i add some tests to check every type, i tested it on sqlite and mysql.