Memoize flattened property lookup #31494
Conversation
|
/cc @danmoseley |
|
OK, if in your judgement risk is low. |
| /// Returns all <see cref="IProperty"/> members from this type and all nested complex types, if any. | ||
| /// </summary> | ||
| /// <returns>The properties.</returns> | ||
| public virtual IEnumerable<Property> GetFlattenedProperties() |
There was a problem hiding this comment.
Should these be public given that they're supposed to be memoized? Or are we doing memoization only for the runtime types?
There was a problem hiding this comment.
Not sure what you mean. They aren't memoized for non runtime properties (because the types returned can change). But even if they were, why does being memoized mean they should be non-public?
There was a problem hiding this comment.
They aren't memoized for non runtime properties (because the types returned can change).
Right, makes sense.
But even if they were, why does being memoized mean they should be non-public?
I just meant that where we memoize, we shouldn't provide public access to the logic that does the costly calculation, bypassing the memoization. All seems ok.
There was a problem hiding this comment.
We should check with @andriipatsula @AndriySvyryd on this. I think I followed the existing pattern on this.
There was a problem hiding this comment.
This is fine. Costly methods won't be available once the model is finalized.
ajcvickers
force-pushed
the
230817_ApePancakes
branch
from
0d5d55d to
6b219eb
Compare
Per #31453 (review)