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

Support for Emoticons Unicode #159

Merged
merged 9 commits into from May 30, 2018

Conversation

3 participants
@mateusfsilva
Copy link
Contributor

mateusfsilva commented Apr 9, 2018

MySQL’s utf8 charset only partially implements proper UTF-8 encoding. It can only store UTF-8-encoded symbols that consist of one to three bytes; encoded symbols that take up four bytes aren’t supported.

Luckily, MySQL 5.5.3 (released in early 2010) https://dev.mysql.com/doc/relnotes/mysql/5.5/en/news-5-5-3.html introduced a new encoding called utf8mb4 which maps to proper UTF-8 and thus fully supports Unicode.

10.9.1 The utf8mb4 Character Set (4-Byte UTF-8 Unicode Encoding) https://dev.mysql.com/doc/refman/5.7/en/charset-unicode-utf8mb4.html

Support for Emoticons Unicode
MySQL’s utf8 charset only partially implements proper UTF-8 encoding. It can only store UTF-8-encoded symbols that consist of one to three bytes; encoded symbols that take up four bytes aren’t supported.

Luckily, MySQL 5.5.3 (released in early 2010) https://dev.mysql.com/doc/relnotes/mysql/5.5/en/news-5-5-3.html introduced a new encoding called utf8mb4 which maps to proper UTF-8 and thus fully supports Unicode.

10.9.1 The utf8mb4 Character Set (4-Byte UTF-8 Unicode Encoding) https://dev.mysql.com/doc/refman/5.7/en/charset-unicode-utf8mb4.html
@tanner0101
Copy link
Member

tanner0101 left a comment

awesome, thanks! Just a couple of comments

static var utf8_general_ci: MySQLCharacterSet = 0x0021
static var binary: MySQLCharacterSet = 0x003f
/// Creates a new `MySQLCharacterSet`
init(string: String) {

This comment has been minimized.

@tanner0101

tanner0101 Apr 16, 2018

Member

Hmm, I think this should be a nil-fallible initializer in case the string is not recognized.

This comment has been minimized.

@mateusfsilva

mateusfsilva Apr 17, 2018

Author Contributor

I agree. But in this case would be better to make MySQLDatabaseConfig throws error.

public init(hostname: String = "127.0.0.1", port: Int = 3306, username: String, password: String? = nil, database: String, characterSet: String = "utf8_general_ci") throws {
  guard let charSet = MySQLCharacterSet(string: characterSet) else {
    throw MySQLError(identifier: "invalidCharacterSet", reason: "Cannot initialize \(MySQLCharacterSet.self) with value \(characterSet).", source: .capture())
  }

  self.hostname = hostname
  self.port = port
  self.username = username
  self.database = database
  self.password = password
  self.characterSet = charSet
}
/// Creates a new `MySQLDatabaseConfig`.
public init(hostname: String = "127.0.0.1", port: Int = 3306, username: String, password: String? = nil, database: String) {
public init(hostname: String = "127.0.0.1", port: Int = 3306, username: String, password: String? = nil, database: String, characterSet: String = "utf8_general_ci") {

This comment has been minimized.

@tanner0101

tanner0101 Apr 16, 2018

Member

why not accept MySQLCharacterSet here if it's public?

This comment has been minimized.

@mateusfsilva

mateusfsilva Apr 17, 2018

Author Contributor

I think is more convinient use String if you get the values from the Environment.

let config = try MySQLDatabaseConfig(hostname: Environment.get("HOSTNAME")!, port: Environment.get("PORT")!, username: Environment.get("USERNAME")!, password: Environment.get("PASSWORD")!, database: Environment.get("DATABASE")!, characterSet: Environment.get("CHARACTERSET")!)
Support for Emoticons Unicode
Change MySQLCharacterSet to nil-fallible initializer.
Change MySQLDAtabaseConfig to throws error.
/// Creates a new `MySQLDatabaseConfig`.
public init(hostname: String = "127.0.0.1", port: Int = 3306, username: String, password: String? = nil, database: String) {
public init(hostname: String = "127.0.0.1", port: Int = 3306, username: String, password: String? = nil, database: String, characterSet: String = "utf8_general_ci") throws {

This comment has been minimized.

@tanner0101

tanner0101 Apr 20, 2018

Member

I think it would be better to expose MySQLCharacterSet instead of string here. If you want to use a String, use the init(string:) method on MySQLCharacterSet. That allows us to make this init non-throwing and let's users use a pre-defined case or even an integer which is more powerful.

This comment has been minimized.

@mateusfsilva

mateusfsilva Apr 21, 2018

Author Contributor

Done

Support for Emoticons Unicode
Changed MySQLDatabaseConfig initializer to non-throwing.
Changed characterSet parameter to MySQLCharacterSet type.
@tanner0101
Copy link
Member

tanner0101 left a comment

Just some gardening stuff, code looks great 👍

static var latin1_swedish_ci: MySQLCharacterSet = 0x0008
static var utf8_general_ci: MySQLCharacterSet = 0x0021
static var binary: MySQLCharacterSet = 0x003f
/// Creates a new `MySQLCharacterSet`

This comment has been minimized.

@tanner0101

tanner0101 Apr 21, 2018

Member

Docblock should include the string parameter. Including some example code on how to use this method would be even better 🙌

}
}

public static var latin1_swedish_ci: MySQLCharacterSet = 0x0008

This comment has been minimized.

@tanner0101

tanner0101 Apr 21, 2018

Member

All public methods need a docblock so that our docs stay 100%.

}

extension MySQLCharacterSet: CustomStringConvertible {
var description: String {
public var description: String {

This comment has been minimized.

@tanner0101

tanner0101 Apr 21, 2018

Member

For this method I would usually just docblock like:

/// See `CustomStringConvertible`.
public var description: String {

That way people know to look at the protocol if they want more information about what the method does.

tanner0101 and others added some commits Apr 21, 2018

Support for Emoticons Unicode
Added docblock for public methods.
Fix #166 - MySQL password bug
Allows users with empty password to log on MySQL. Fix #166

@mateusfsilva mateusfsilva force-pushed the mateusfsilva:master branch from 42d8b4f to f196c53 Apr 23, 2018

mateusfsilva added some commits Apr 27, 2018

Revert "Fix #166 - MySQL password bug"
This reverts commit 42d8b4f.
@mateusfsilva

This comment has been minimized.

Copy link
Contributor Author

mateusfsilva commented Apr 27, 2018

I added the docblock to the public methods.

/// Creates a new `MySQLCharacterSet` from a string. Can return nil if the string is a invalid or unsupported collation.
///
/// - Parameter string: Collation name of desirable character set.
///
/// Currently accepting the followings collations:
/// * latin1_swedish_ci
/// * utf8_general_ci
/// * binary
/// * utf8mb4_unicode_ci
///
/// Example: MySQLCharacterSet(string: "utf8mb4_unicode_ci")
public init?(string: String) {
    switch string {
    case "latin1_swedish_ci": self.raw = 0x0008
    case "utf8_general_ci": self.raw = 0x0021
    case "binary": self.raw = 0x003f
    case "utf8mb4_unicode_ci": self.raw = 0x00e0
    default: return nil
    }
}
/// `MySQLCharacterSet` value.
/// * Collation: latin1_swedish_ci
/// * Character set: latin1
/// * Id: 8
/// * Value: 0x0008
public static var latin1_swedish_ci: MySQLCharacterSet = 0x0008
/// `MySQLCharacterSet` value.
/// * Collation: utf8_general_ci
/// * Character set: utf8
/// * Id: 33
/// * Value: 0x0021
public static var utf8_general_ci: MySQLCharacterSet = 0x0021
/// `MySQLCharacterSet` value.
/// * Collation: binary
/// * Character set: binary
/// * Id: 63
/// * Value: 0x003f
public static var binary: MySQLCharacterSet = 0x003f
/// `MySQLCharacterSet` value.
/// * Collation: utf8mb4_unicode_ci
/// * Character set: utf8mb4
/// * Id: 224
/// * Value: 0x00e0
public static var utf8mb4_unicode_ci: MySQLCharacterSet = 0x00e0
/// See `CustomStringConvertible`.
public var description: String {

@tanner0101 tanner0101 added this to the 3.0.0-rc.2.3 milestone May 30, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented May 30, 2018

thanks!

@penny-coin

This comment has been minimized.

Copy link

penny-coin commented May 30, 2018

Hey @mateusfsilva, you just merged a pull request, have a coin!

You now have 1 coins.

@tanner0101 tanner0101 merged commit 45266fe into vapor:master May 30, 2018

3 of 5 checks passed

ci/circleci: 8.0-linux-fluent Your tests failed on CircleCI
Details
ci/circleci: 8.0-linux CircleCI is running your tests
Details
ci/circleci: 5.5-linux Your tests passed on CircleCI!
Details
ci/circleci: 5.7-linux Your tests passed on CircleCI!
Details
ci/circleci: linux-release Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment