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

Expandable collection support. #143

Merged
merged 3 commits into from
Dec 18, 2021
Merged

Expandable collection support. #143

merged 3 commits into from
Dec 18, 2021

Conversation

Andrewangeta
Copy link
Member

Fixes #117

@Andrewangeta Andrewangeta requested a review from gwynne December 18, 2021 01:40
gwynne
gwynne previously approved these changes Dec 18, 2021
Comment on lines 183 to 196
do {
let container = try decoder.singleValueContainer()
do {
if container.decodeNil() {
_state = .empty
} else {
_state = .unexpanded(try container.decode([String].self))
}
} catch DecodingError.typeMismatch(let type, _) where type is String.Type {
_state = .expanded(try container.decode([Model].self))
}
} catch DecodingError.keyNotFound(_, let context) where context.codingPath.count == codingPath.count {
_state = .empty
}
Copy link
Member

Choose a reason for hiding this comment

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

Define this extension somewhere (in this file if you like), or elsewhere:

internal extension Decoder {
    func singleValueContainerIfPresentAndNotNull() throws -> SingleValueDecodingContainer? {
        do {
            let container = try self.singleValueContainer()
            
            if container.decodeNil() {
                return nil
            }
            return container
        }
        catch DecodingError.keyNotFound(_, let context)
            where context.codingPath.count == self.codingPath.count
        {
            return nil
        }
    }
}

(n.b.: I tinkered with the code formatting for the catch part about a dozen times, I just can't figure out something that feels idiomatic for that 😅)

Then you can do this and avoid some of the convoluted boilerplate:

Suggested change
do {
let container = try decoder.singleValueContainer()
do {
if container.decodeNil() {
_state = .empty
} else {
_state = .unexpanded(try container.decode([String].self))
}
} catch DecodingError.typeMismatch(let type, _) where type is String.Type {
_state = .expanded(try container.decode([Model].self))
}
} catch DecodingError.keyNotFound(_, let context) where context.codingPath.count == codingPath.count {
_state = .empty
}
if let container = try decoder.singleValueContainerIfPresentAndNotNull() {
do {
self._state = .unexpanded(try container.decode([String].self))
} catch DecodingError.typeMismatch(let type, _) where type is String.Type {
self._state = .expanded(try container.decode([Model].self))
}
} else {
self._state = .empty
}

Obviously you can do the same in @Expandable as well.

(Disclaimer: There are no functional changes in this code versus your version; it's entirely a stylistic choice. I personally don't like nested do blocks, especially when the errors being explicitly caught are as complex as DecodingErrors usually are. And of course there's always something to appreciate in factoring out reused logic. Codable's API makes things hard enough to read already.)

gwynne
gwynne previously approved these changes Dec 18, 2021
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

LGTM!

I keep wanting to write this rant about how you should make the ExpandableState enums Codable and put the switches in those decode/encode methods instead of on the property wrappers (which could then let the compiler synthesize their conformance), and I keep having to remind myself that it actually doesn't change anything whatsoever so why not keep the enums clean? 🤣 But yeah, this seems great, ship it! 💯

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Darn auto-dismiss 😆

@Andrewangeta Andrewangeta merged commit e1b4bae into main Dec 18, 2021
@Andrewangeta Andrewangeta deleted the expandable-collection branch December 18, 2021 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

@Expandable used with array of Ids.
2 participants