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

add decoding for UUID #1137

Merged
merged 8 commits into from
Jun 29, 2022
Merged

add decoding for UUID #1137

merged 8 commits into from
Jun 29, 2022

Conversation

atultw
Copy link
Contributor

@atultw atultw commented Jun 10, 2022

I stumbled upon issue #1126 while trying to decode a UUID using codable. As described there, encoding worked fine but decoding treated the field like json.

Added an else to cover the UUID case, matching the encode method functionality.

@atultw
Copy link
Contributor Author

atultw commented Jun 14, 2022

lint should pass now

@jberkel
Copy link
Collaborator

jberkel commented Jun 14, 2022

Thanks for your contribution. Can you add some tests?

@atultw
Copy link
Contributor Author

atultw commented Jun 15, 2022

Added a UUID field to TestCodable. I figure that's the best way since it already has fields for date and other types.

I copy pasted apple's example UUID from foundation docs for the tests. May want to make it a constant instead. Let me know what you think!

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

As the number of branches get bigger, it might be better to use a switch statement here:

switch type {
case is Data.Type:
...

The same actually for encode:

func encode<T>(_ value: T, forKey key: Key) throws where T: Swift.Encodable {
    switch value {
    case let data as Data:
    ...

builder.column(Expression<String?>("optional"))
builder.column(Expression<Data>("sub"))
})

let value1 = TestCodable(int: 1, string: "2", bool: true, float: 3, double: 4,
date: Date(timeIntervalSince1970: 0), optional: nil, sub: nil)
date: Date(timeIntervalSince1970: 0), uuid: UUID(uuidString: "E621E1F8-C36C-495A-93FC-0C247A3E6E5F")!, optional: nil, sub: nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be defined once in a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to TestHelpers

@atultw
Copy link
Contributor Author

atultw commented Jun 27, 2022

Thanks Jan. Made the changes, tests passing. I put the uuid constant in TestHelpers, can move it elsewhere if needed

@jberkel
Copy link
Collaborator

jberkel commented Jun 28, 2022

Ok, looking good, just a few lint fixes left.

@atultw
Copy link
Contributor Author

atultw commented Jun 28, 2022

Tried splitting the lines that were too long into two lines, but that made the test fail. So I disabled the rule at those spots.

""".replacingOccurrences(of: "\n", with: ""),
insert
)
// swiftlint:enable line_length
Copy link
Collaborator

@jberkel jberkel Jun 29, 2022

Choose a reason for hiding this comment

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

you shouldn't need the swiftlint statements here. when you break the string, ensure that there's a single space at the beginning of the row, and no whitespace at the end of each row:

assertSQL(
    """
    INSERT INTO \"emails\" (\"int\", \"string\", \"bool\", \"float\", \"double\", \"date\", \"uuid\", \"optional\",
     \"sub\") VALUES (1, '2', 1, 3.0, 4.0, '1970-01-01T00:00:00.000', 'E621E1F8-C36C-495A-93FC-0C247A3E6E5F',
     'optional', '\(encodedJSONString)')
    """.replacingOccurrences(of: "\n", with: ""),

Note the single space (relative to the string start marker """) before \"sub\" and 'optional'

return uuid as! T
default:
// swiftlint:enable force_cast
guard let JSONString = try row.get(Expression<String?>(key.stringValue)) else {
Copy link
Collaborator

@jberkel jberkel Jun 29, 2022

Choose a reason for hiding this comment

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

variables should be lowercase actually, not even your changes, ignore

@jberkel jberkel merged commit 55f4565 into stephencelis:master Jun 29, 2022
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