Refactor DB migrations types to be less MySQL-specific #546
Conversation
Notes on these changes: * Column types are now interfaces, rather than enums, to allow type-specific customisation * The size and default value for a column has now been included as part of the type interfaces, to avoid cases that make very little sense (like DATETIME(1)) * The smallint and int types have been merged into a single integer type, which can also emit tinyint, mediumint, and bigint for MySQL * The "current timestamp" default value is now part of a datetime column type, instead of being a weird hack on top of the column * The SQLBuilder interface is now in its own module, since it being specifically in the create module was misleading The existing migrations which use these types have been modified to accomodate the refactor, which also helps serve as examples for the existing types. The primary purpose of this refactor is to better accomodate additional database dialects, namely PostgreSQL.
The PostgreSQL dialect isn't usable right now, but this is mostly for testing to make sure everything would work. The biggest change required here is that PostgreSQL has no equivalent to the MySQL "change column" syntax, and must make each change to columns individually.
|
This is amazing! (big fan of Postgres), I guess the PR is a bit big like you said and that's probably why it's not been looked at/merged yet? Even if this doesn't get merged in, I still want to give kudos to the work done here! (also, useful for other forks that might want Postgres support) |
|
I'll be honest, after working a lot with the code to try and help implement stuff, I got kind of burnt out by the code's overall quality in addition to it being written in golang, which further discourages code quality imho. (golang encouraging bad code, I mean) I don't blame the folks who wrote it for this, and I'm sure that they just have other stuff going on, but the combination of messy code + no resources to really work with the community on the codebase has mostly pushed me toward eventually wanting to write my own version in Rust. The requirement to sign away the licensing agreements for contributions also puts an extremely sour taste in my mouth, since I expect that garbage of big industry players like Microsoft, but not from smaller orgs who should work with their communities instead. So, while I'd be happy to see changes like this get merged & more stuff implemented, I'm not really counting on it. There's tonnes of stuff to be said about actually making something instead of vaguely hinting to wanting to like I am, but, I figured I'd at least pitch in with how I feel after drafting such a big change. |
|
I appreciate you taking the time to develop this and share your thoughts @clarfonthey, and sorry for not responding earlier! I absolutely want to support Postgres in WF. My only concern is with ongoing support for another database backend, especially since I'm the only one maintaining this project. If you or anyone else can commit to maintaining it, then we can move forward with this. Btw if you're interested in working with the community on anything in WF, the Contributing guide mentions that our forum is the best place to organize that. Again I'm trying to work with the community but I'm the only one regularly working on this, so we need anyone interested in seeing the project improve to step forward and help coordinate that. |
|
As mentioned, I don't think I'll be able to actually maintain that, since I don't know if I'll be able to dedicate myself to this. That said, when you have time to look into this, the PR as it's presented should be a strict improvement on the existing code, and also cleans up the MySQL side as well. I think it's fair to not commit to maintaining a PostgreSQL interface since that involves all the extra testing, but at least modifying the existing dialect code to be less MySQL-specific should hopefully improve the situation so maybe someone else can take over. |
Notes on these changes:
PostgreSQLdialect is added that is by no means ready to use, but is helpful to show how these would be adapted to PostgreSQLName,Default,CreateSQL, andAlterSQLThe existing migrations which use these types have been modified to accomodate the refactor, which also helps serve as examples for the existing types.
The primary purpose of this refactor is to better accomodate additional database dialects, namely PostgreSQL.
I should also apologise for dropping a mid-size PR unannounced, although after seeing how relatively simple the DB code was, I felt inspired to take on adding PostgreSQL support since it doesn't seem like anyone else has done work on that for a while. I understand that this may take a bit to get to as a result!