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.

Sign up for GitHub

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

Jump to bottom

Add optional support for decoding #1224

Merged
merged 4 commits into from Oct 23, 2023
Merged

Add optional support for decoding #1224

merged 4 commits into from Oct 23, 2023

Conversation

JacobHearst
Copy link
Contributor

Closes #997 by adding support for optional types when decoding a row.

@JacobHearst
Copy link
Contributor Author

The test_insert_encodable_with_nested_encodable() test is failing although truthfully I'm not sure how it ever passed since it seems to relies on the order of keys in a dictionary. Am I missing something?

@nathanfallet
Copy link
Collaborator

I'm going to take a look to tests, but that's not normal if it fails... (also you might need to add tests to cover your new methods)
I was wondering, why is your generic method not called with the suffix ifPresent as well? There is already a generic decode method just before your addition.

@nathanfallet nathanfallet self-requested a review October 17, 2023 17:54
@JacobHearst
Copy link
Contributor Author

JacobHearst commented Oct 18, 2023 •
edited

I was wondering, why is your generic method not called with the suffix ifPresent as well? There is already a generic decode method just before your addition.

An oversight on my part (too much copypasta).

also you might need to add tests to cover your new methods

The SQLiteEncoder and SQLiteDecoder classes are private, are you okay with making them internal to allow for testing?

nathanfallet
nathanfallet reviewed Oct 19, 2023
View reviewed changes
Sources/SQLite/Typed/Coding.swift Outdated
func decodeIfPresent<T>(_ type: T.Type, forKey key: Key) throws -> T? where T: Swift.Decodable {
switch type {
case is Data.Type:
if let data = try? row.get(Expression<Data>(key.stringValue)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can write all those lines as return try? ... as? t directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah not sure what was going though my head here

@nathanfallet
Copy link
Collaborator

If they are private that means they are not tested at all... that's scary.
I think making them internal and add tests on them could be an all new PR, so feel free to make it if you want to.

@JacobHearst
Copy link
Contributor Author

I think making them internal and add tests on them could be an all new PR, so feel free to make it if you want to.

I think I'll open up an issue for it if that's alright? I may still come back and open a PR to close that issue but likely not anytime soon

@nathanfallet nathanfallet merged commit 3d25271 into stephencelis:master Oct 23, 2023
2 checks passed
@JacobHearst JacobHearst mentioned this pull request Oct 23, 2023
Open
@AnthonyMDev AnthonyMDev mentioned this pull request Jan 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanfallet nathanfallet nathanfallet left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SQLiteDecoder & Codable with NULL Values
2 participants
@JacobHearst @nathanfallet