SequenceAllSatisfy is mapped to SequenceContainsWhere, and not SequenceAllSatisfy#1626
SequenceAllSatisfy is mapped to SequenceContainsWhere, and not SequenceAllSatisfy#1626SiliconA-Z wants to merge 1 commit intoswiftlang:mainfrom
Conversation
|
@swift-ci please test |
…ceAllSatisfy This is a typo. SequenceAllSatisfy should map to SequenceAllSatisfy.
Thank you. I fixed it. Can you run it one more time. |
| configuration.allowPartialType(PredicateExpressions.SequenceContains<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Int>>.self, identifier: "PredicateExpressions.SequenceContains") | ||
| configuration.allowPartialType(PredicateExpressions.SequenceContainsWhere<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Bool>>.self, identifier: "PredicateExpressions.SequenceContainsWhere") | ||
| configuration.allowPartialType(PredicateExpressions.SequenceContainsWhere<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Bool>>.self, identifier: "PredicateExpressions.SequenceAllSatisfy") | ||
| configuration.allowPartialType(PredicateExpressions.SequenceAllSatisfy<PredicateExpressions.Value<[Int]>, PredicateExpressions.Value<Bool>>.self, identifier: "PredicateExpressions.SequenceAllSatisfy") |
There was a problem hiding this comment.
Hmm, we might have a compatibility issue here that we need to address. I believe as it stands today, this registers SequenceContainsWhere with both the PredicateExpressions.SequenceContainsWhere and PredicateExpressions.SwquenceAllSatisfy identifiers. PredicateCodableConfiguration will take the last identifier registered when the same type is registered with multiple identifiers, so today I think we've been encoding/decoding PredicateExpressions.SequenceContainsWhere with the PredicateExpressions.SequenceAllSatisfy identifier. Changing this would break any existing archives that already have a serialized SequenceContainsWhere stored under the SequenceAllSatisfy identifier. It would also break sending a serialized archive from a newer runtime to an application that deserializes it with an older runtime.
I'm not sure how many of those archives exist today in the wild. We can try to quantify that, but we might need to instead think about a compatible way to move forward even if these identifiers are incorrect so that we can solve the problem (allowing serialization of SequenceAllSatisfy) without breaking existing serializations of SequenceContainsWhere. Perhaps we leave a comment here about the unfortunate identifier mismatch and use a new identifier for the real SequenceAllSatisfy type?
There was a problem hiding this comment.
Are you okay with the new identifier route
There was a problem hiding this comment.
Yeah that's probably the best way to go. Let's update the SequenceContainsWhere line above to use the incorrect identifier (with a comment explaining why the identifier doesn't match) and then for SequenceAllSatisfy we can come up with a new identifier to use.
In terms of a new identifier since the appropriate one is "taken" in existing archives, maybe something like PredicateExpressions.SequenceAllSatisfy.corrected? @parkera @itingliu do you have any suggestions? I'm not sure if we've had a pattern before around naming and evolving archive keys that were incorrect historically that we should follow here.
There was a problem hiding this comment.
I think PredicateExpressions.SequenceAllSatisfy.corrected is fine. I think sometimes we also add the version number into the coding key as a way to document when the new key goes into effect, like PredicateExpressions.SequenceAllSatisfy_6_4. But I don't have strong opinions on which we choose
There was a problem hiding this comment.
Sounds good - @AZero13 how about we go with PredicateExpressions.SequenceAllSatisfy.corrected as the identifier then? And let's leave a comment here explaining why that's necessary
3 participants
This is a typo. SequenceAllSatisfy should map to SequenceAllSatisfy.