-
Notifications
You must be signed in to change notification settings - Fork 26
Attempt to make ResilientDecoding work with Swift Concurrency #53
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
base: master
Are you sure you want to change the base?
Conversation
dfed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a reasonable approach to me. I'm curious how hard this change would be to adopt for consumers, though I'm not sure that should stop y'all from rolling a breaking change like this.
| The raw value for which `init(rawValue:)` returned `nil`. | ||
| */ | ||
| public let novelValue: Any | ||
| public let novelValue: Sendable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that this is a breaking change, and we should update the sample dependency management code in the README to match. Podspec needs bumping too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, I see this being a minor version bump at a minimum.
|
I had a thought earlier this morning about trying to make this work via conditional conformance instead of requiring all values to be Edit: Not feasible due to all the internal generic objects used in DEBUG builds. Might be possible for RELEASE but I think enforcing |
| onlyGenerateCoverageForSpecifiedTargets = "YES"> | ||
| <CodeCoverageTargets> | ||
| <BuildableReference | ||
| BuildableIdentifier = "primary" | ||
| BlueprintIdentifier = "ResilientDecoding" | ||
| BuildableName = "ResilientDecoding" | ||
| BlueprintName = "ResilientDecoding" | ||
| ReferencedContainer = "container:"> | ||
| </BuildableReference> | ||
| </CodeCoverageTargets> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensures code coverage numbers are accurate - previously was including the test target coverage too.
| The raw value for which `init(rawValue:)` returned `nil`. | ||
| */ | ||
| public let novelValue: Any | ||
| public let novelValue: Sendable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, I see this being a minor version bump at a minimum.
|
Hey, major bump is technically correct since the version would contain breaking changes, but since this library is perfect as it is and hasn't needed any releases in a quite a long time a minor one is fine as well IMO, though that's up to @dfed. @bdbergeron could you please update the version in ResilientDecoding.podspec? Also, @dfed what changes do you envision for the README, info about the new version requiring By the way, since I'm already writing this, I want to thank you @dfed for this wonderful utility and @bdbergeron for the preparation for strict Swift concurrency. ❤️ |
Starting a conversation about updating this library to support Swift Concurrency, namely
Sendablevalues, so consumers don't have to use@preconcurrecny import ResilientDecoding. This simple "build and fix" approach may not be the most ideal, as it requires the wrapped property to no longer simply be aDecodablebut also aSendablevalue. However, it's worth having the conversation of what the future of this library looks like in a modern Swift world, so here I'm proposing the first steps to getting us to a more modern Swift syntax.