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

db: fix unicode on mysql #1652

Merged
merged 1 commit into from
Jul 3, 2019
Merged

db: fix unicode on mysql #1652

merged 1 commit into from
Jul 3, 2019

Conversation

RustyBower
Copy link
Contributor

No description provided.

@deathbybandaid
Copy link
Contributor

After some testing, I can confirm this clears issue #1650

@RustyBower
Copy link
Contributor Author

I did just configure this for a single column in nick_values, but we should chat about all the potentially places people could put unicode characters and configure appropriately.

@deathbybandaid
Copy link
Contributor

I would imagine this should be added to channel_values,, as well as the upcoming plugin_values in PR #1621

@dgw
Copy link
Member

dgw commented Jul 2, 2019

What @deathbybandaid said. Fixing this for only one DB column is no good; we need to allow Unicode pretty much anywhere there's a string/text. Some IRCds do in fact allow Unicode nicknames, for example.

@dgw dgw added Bugfix Generally, PRs that reference (and fix) one or more issue(s) High Priority labels Jul 2, 2019
@dgw dgw added this to the 7.0.0 milestone Jul 2, 2019
@RustyBower RustyBower force-pushed the db_unicode_fix branch 4 times, most recently from 0ea5981 to ff366c9 Compare July 2, 2019 19:41
sopel/db.py Outdated Show resolved Hide resolved
@deathbybandaid
Copy link
Contributor

Fresh database with mysql seems good with the __table_args__ setup.
I will try testing with sqlite and postgres later today.

I think those 3 databases are the only ones we should focus on. Any other database flavors can wait until people have issues with them.

@deathbybandaid
Copy link
Contributor

Should we be concerned about line length with this?

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

@RustyBower Can we apply this also to all the tables it makes sense to? Channel names are very often Unicode-enabled even if nicknames aren't, for example.

I think we need this on almost everything:

  • ChannelValues
  • NickValues
  • Nicknames

I guess NickIDs doesn't need it, because integers aren't strings. 😝

Alternatively, if you can set the default engine, charset, and collation for the whole database somehow, do that. 😈

@RustyBower
Copy link
Contributor Author

Fixed the line length and additional table issues. I'll continue to do some research into defaulting this for the entire table.

@RustyBower RustyBower changed the title db: allowing unicode nick_values db: allowing unicode nick_values, nicknames, channel_names Jul 3, 2019
@RustyBower RustyBower changed the title db: allowing unicode nick_values, nicknames, channel_names db: fixing db unicode support Jul 3, 2019
@dgw dgw changed the title db: fixing db unicode support db: fix unicode on mysql Jul 3, 2019
@RustyBower RustyBower changed the title db: fix unicode on mysql db: fix db unicode support Jul 3, 2019
@dgw dgw changed the title db: fix db unicode support db: fix unicode on mysql Jul 3, 2019
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Much ❤️ to @RustyBower for amending this so many times based on my feedback!

Because I've already asked him to force-push so many times, I'll ignore the somewhat unnecessary change to line-breaks in the URL() call. It's fine~!

@dgw dgw merged commit f8c287c into sopel-irc:master Jul 3, 2019
@dgw
Copy link
Member

dgw commented Jul 3, 2019

(And I won't give Rusty time to "fix it anyway", either. 😈)

@Exirel
Copy link
Contributor

Exirel commented Jul 4, 2019

Awesome fix! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) High Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants