Dynamic bridge app PR introduced weird layering violation

[bzbarsky-apple]

#21975 added a WillDecodeNull method on AttributeValueDecoder that doesn't really make sense. AttributeValueDecoder should just be used to decode into the canonical data model types; if you need the result converted to some other type, then you can do it from those.

In particular, there is logic for "decoding lists as spans" that seems to require this WillDecodeNull bit but doesn't make any sense to me....

@mrjerryjohns @achaulk-goog

Also, I don't understand why #21975 was fast-tracked given it was making core API changes. @woody-apple @andy31415

[andy31415]

Fast track was because PR has been in review for about 7 weeks give or take - after so much time, even considering 1.0 crunch, it was likely that nobody else cared enough to actually review it :/

Since you did review it in the end, that assumption seems to maybe be wrong and maybe 1.0 crunch should have stayed it more. It seems we have to adjust logic about null decoding. @mrjerryjohns as primary reviewer.

[mrjerryjohns]

Had a discussion with @bzbarsky-apple and clarified the rationale there. There are a number of small bugs that were introduced in the bridge-app side of things that @achaulk-goog is going to put out a PR for.

As for the usage of WillDecodeNull, it was the simplest strategy to take to hit the use-case needed for introducing these templated 'container/collections' types. It is not breaking any existing APIs, and just adds one extra method.

Alternative strategies might be much more complicated, so @bzbarsky-apple is going to help think of those once @achaulk-goog 's PR is out.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.