Skip to content

Conversation

adamchainz
Copy link
Member

Copy link
Member

Choose a reason for hiding this comment

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

Missing self :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, woops, I wrote this on a train without a working local mariadb so didn't run tests

@adamchainz
Copy link
Member Author

I haven't yet tested this on MariaDB 10.2. Also this brings up the question of whether Django CI should start testing on MariaDB as well as MySQL 😉

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a feature flag and to allow a Maria DB specific backend to override it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think adding a MariaDB backend would be a good idea eventually but first I'd like to get Django using pymysql as well, which I have half-done at https://github.com/adamchainz/django-pymysql-backend . But then we'd possibly have 2 MySQL backends and 2 MariaDB backends inheriting from them 😞 .

Copy link
Member

@timgraham timgraham left a comment

Choose a reason for hiding this comment

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

I guess it's fine provided you test it.

match = server_version_re.match(server_info)
return cursor.fetchone()[0]

@property
Copy link
Member

Choose a reason for hiding this comment

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

Should the new properties be cached?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe mysql_version but mysql_is_mariadb is very simple

treats these columns as nullable.
treats these columns as nullable. MariaDB 10.2+ fixes this.
"""
if self.connection.mysql_is_mariadb and self.connection.mysql_version[:2] > (10, 2):
Copy link
Member

Choose a reason for hiding this comment

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

I think [:2] is unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it is, just a habit


def skip_default(self, field):
"""
MySQL doesn't accept default values for some data types and implicitly
Copy link
Member

Choose a reason for hiding this comment

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

I'd say, "MySQL (except MariaDB 10.2+) doesn't ..."

@timgraham
Copy link
Member

Adam, do you want to finish this up?

@adamchainz
Copy link
Member Author

I do, but I was thinking of forming a better plan around MariaDB first...

@timgraham
Copy link
Member

Reopen this when you return to it?

@timgraham timgraham closed this Jun 1, 2017
@adamchainz
Copy link
Member Author

adamchainz commented Jun 2, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants