Closed ericlewis closed 3 years ago
Hi @ericlewis,
We appreciate your submission of this PR! We actually have some internal changes to fix these warnings on the way to release currently. The structs with default fields will never actually need to be decoded in SDK use, so the approach we've done internally is to remove the derived Decodable
protocol conformance leaving the structs only Encodable
. This also requires some test changes as there are some tests for encoding/decoding of these structs, which are especially overboard given we never decode any of the ones with constant properties. The DiagnosticEvent
models were the first significant place to use the Codable
protocols in the SDK, so the tests were mainly to improve my understanding.
I agree that your approach here is cleaner than providing construction arguments for the fields, but I'm leaning towards the internal changes to keep these fields as let
s and remove the redundant testing code. Happy to hear any feedback on that approach as well though, we on the SDK team have to wear a lot of hats so we're not always domain experts on the specific platforms and styles.
Thanks for the very thoughtful PR, we really value external contributions to our SDKs. We should be able to roll out a release with these warnings fixed soon.
Thanks again, @gwhelanLD
@gwhelanLD I was exactly curious if it needed to be Decodable 💯 I would think that is certainly the better solution. Closing this for now.
Requirements
Related issues
220
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen. This PR fixes the warnings mentioned in #220, which were added to the Swift compiler somewhat recently. The cause of the warning is setting default values for a Decodable objects properties. This causes subtle bugs where a given property may not be correctly decoded.
The solution proposed here is one of a few, simply settings the variable to a
var
while making its setter private. This retains the original behavior for those consuming the code, while also allowing Decodable to work properly.Describe alternatives you've considered
Another option would be to inject the default values when
init
is called, but that adds more arguments and some models do not have initializers to begin with, this is a smaller change. Such a change would look like this:This seems possibly like something we don't really want, because it opens up the user to being able to change a value that seems like it should remain static.
Additional context
This is the background for the warning: https://github.com/apple/swift/pull/30218