Skip to content
This repository was archived by the owner on Jul 7, 2021. It is now read-only.

Add mysql support #41

Merged
merged 18 commits into from
Jan 2, 2019
Merged

Conversation

rippinrobr
Copy link
Collaborator

Copied functionality from Postgres and updated it for MySQL. I've used it to create tables and columns as well as dropping tables. I also copied the SQLite tests and updated them for MySQL. I've run the code against two data sets which create about 50 tables combined. Open to any and all feedback.

@coveralls
Copy link

Coverage Status

Coverage decreased (-10.6%) to 61.286% when pulling e7ba5f8 on rippinrobr:add-mysql-support into 539b9da on spacekookie:master.

@coveralls
Copy link

coveralls commented Dec 29, 2018

Coverage Status

Coverage decreased (-0.05%) to 71.878% when pulling 90a32b5 on rippinrobr:add-mysql-support into 539b9da on spacekookie:master.

Copy link
Collaborator

@spacekookie spacekookie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, thanks for the PR ❤️

There's a few small comments I have, and also there should probably be more tests in the future (sad coveralls bot is sad apparently).

But I don't see that as a blocker to merge the feature already!

Integer => format!("{}{} {}", MySql::prefix(ex), name, MySql::print_type(bt)),
Float => format!("{}{} {}", MySql::prefix(ex), name, MySql::print_type(bt)),
Double => format!("{}{} {}", MySql::prefix(ex), name, MySql::print_type(bt)),
UUID => unimplemented!(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the MySQL 8.0 docs there are supported UUID fields. This is definitely something that barrel should reflect.

Unrelated: this also makes me wonder about how to document the vast amounts of implementation specific features across multiple versions as well 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add support for UUID for sure. As far as documentation goes, that is a great question, to which, I don't have a great answer. I can look around to see if there are others who've done it well and if not I'm willing to help in that cause.

src/tests/mod.rs Outdated
#[cfg(feature = "pg")]
mod pg;

#[cfg(feature = "sqlite3")]
mod sqlite3;
mod sqlite3;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about this change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As am I. I'm not sure why github is showing this change, which is a removal of a newline character at the end of that line and not the addition of the mod mysql lines. I can add the newline back in if you'd like.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice, yes 😅

Rob Rowe and others added 2 commits December 31, 2018 11:49
…s backwards compatible. MySQL 8.0 has support for VARBINARY(16)
@spacekookie spacekookie merged commit 66dcd4d into rust-db:master Jan 2, 2019
@JasonMiesionczek
Copy link
Contributor

When will a version be tagged that includes this? Thanks.

@spacekookie
Copy link
Collaborator

Ha! I almost forgot about this. You're right, a new version should have been published weeks ago. Unfortunately I got caught up with a massive yakshave, including this cargo pr and cargo-release pr. The former has been merged, the latter isn't finished yet.

But I'll publish a new version, probably tonight. Thanks for reminding me!

@spacekookie
Copy link
Collaborator

spacekookie commented Jan 21, 2019

@JasonMiesionczek I just published 0.5.0-rc.1. This does however also include a bunch of breaking changes around types. Do check out the up to date docs on docs.rs!

@JasonMiesionczek
Copy link
Contributor

@spacekookie great, thanks! i've already been using the master branch, so i should be ok with the changes. I'll be testing this in earnest either later today or tomorrow, so i'll report back if i find any issues.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants