Skip to content
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

CreateTableSuffix add DEFAULT #432

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from

Conversation

jinggangnanyou
Copy link

No description provided.

@nelsam
Copy link
Member

nelsam commented Sep 1, 2022

Can you add a little bit of information about why this is needed? I don't personally use mysql for anything, so I want to make sure that this won't break anything for anyone else.

Also, this needs tests before it can be merged.

@jinggangnanyou
Copy link
Author

In MySQL reference manual 5.7:

_CREATE {DATABASE | SCHEMA} [IF NOT EXISTS] db_name
[create_option] ...

create_option: [DEFAULT] {
CHARACTER SET [=] charset_name
| COLLATE [=] collation_name
}_

"DEFAULT" keyword is optional, but in some specific version which I am using, it is required. So I think it is more suitable to add "DEFAULT" keyword to support all the scene.

@nelsam
Copy link
Member

nelsam commented Sep 7, 2022

Can you be certain that all versions of mysql allow the DEFAULT keyword? I worry that this could break someone else who might be on a version of mysql that doesn't allow it. We may need to add this as an option to the constructor to ensure that we don't break backward compatibility.

Also, this does still need tests.

@jinggangnanyou
Copy link
Author

Can you be certain that all versions of mysql allow the DEFAULT keyword? I worry that this could break someone else who might be on a version of mysql that doesn't allow it. We may need to add this as an option to the constructor to ensure that we don't break backward compatibility.

Also, this does still need tests.

Yes, for all the mysql version: 5.6, 5.7, 8.0, allow the DEFAULT keyword. But, it is more suitable to add this as an option to the constructor certainly.

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.

2 participants