Polymorphic interfaces #204
Conversation
| /** | ||
| * @member {string} fishtype Polymorphic Discriminator | ||
| */ | ||
| fishtype: "Fish"; |
There was a problem hiding this comment.
I inserted a .OrderBy in the generator to make the discriminator always the first property in the polymorphic interface. I think this is a better user experience.
src/vanilla/Model/CompositeTypeTS.cs
Outdated
| } | ||
|
|
||
| /// <summary>The name to use when referring to this type in a parameter, return type or property definition.</summary> | ||
| public string ReferenceName => PolymorphicUnionCases.Any() ? Name + "Union" : Name.ToString(); |
There was a problem hiding this comment.
Maybe call this UnionTypeName? It's basically saying that if this type is part of union, then use this type's name with the "Union" suffix. Otherwise, just use this type's name.
There was a problem hiding this comment.
Might make sense. I thought the name conveyed the purpose: when referencing this type in an annotation, use this ReferenceName, and when declaring it or referencing it in a mapper, use Name. But it's a little contrived no matter what you do.
src/vanilla/Model/CompositeTypeTS.cs
Outdated
| CodeModel.ModelTypes | ||
| .Where(m => m.BaseModelType == this) | ||
| .Cast<CompositeTypeTS>() | ||
| .ToList(); |
There was a problem hiding this comment.
So this only looks for types that derive from this type, right? This won't include this type?
There was a problem hiding this comment.
Yeah, this is so you can type FishUnion = Fish | SalmonUnion | SharkUnion-- by treating "this type" as a special case, you can make sure you reference the interface instead of the tagged union.
There was a problem hiding this comment.
Think this would be better named... ImmediatePolymorphicSubtypes or something..
src/vanilla/Model/CompositeTypeTS.cs
Outdated
| @@ -33,6 +33,35 @@ public override Property Add(Property item) | |||
| return result; | |||
| } | |||
|
|
|||
| private List<CompositeTypeTS> polymorphicUnionCases; | |||
There was a problem hiding this comment.
Why is this a List? Is it going to be modified after it is initially created?
There was a problem hiding this comment.
I'd like for it to be immutable, computed lazily on the first get and then cached. This is used in ReferenceName to decide whether to refer to the type as FooUnion or just Foo based on whether any polymorphic subtypes exist.
There was a problem hiding this comment.
I get that, but why is it a List and not an Array or an IEnumerable?
There was a problem hiding this comment.
Not IEnumerable because I want it to be clear that "work" isn't being done upon enumeration.
There was a problem hiding this comment.
If that's the case, then why not an array? Also, why does the laziness of the work matter to you? Isn't your only requirement that it be a sequence of values?
There was a problem hiding this comment.
It can be an array. We can convey even more that you're not meant to overwrite values in the array by using IReadOnlyList. I just want to avoid repeated execution of the query through multiple usages of ReferenceName.
src/vanilla/Model/CompositeTypeTS.cs
Outdated
| private List<CompositeTypeTS> polymorphicUnionCases; | ||
|
|
||
| /// <summary>Contains the immediate polymorphic children of this class.</summary> | ||
| public List<CompositeTypeTS> PolymorphicUnionCases |
There was a problem hiding this comment.
Why is this a List? Are you expecting the caller of this property to add or remove values from this List?
There was a problem hiding this comment.
Same as above. Why isn't this an IEnumerable?
|
Updated a few things based on feedback--see what you think. I'm not adamantly against the PolymorphicUnionCases (which I maybe should rename) being array or IEnumerable, just want to avoid a trap I've gotten into in the past where an expensive query I enumerated multiple times slowed things down. |
|
I understand your performance concern. My biggest frustration with AutoRest is that the model that is presented passes through several layers that modify the original swagger specification, and it's difficult for me to keep track of what changes occurred where. I lean more towards immutable models that the generator can do queries and computations on and then store the results of those queries and computations in local variables for performance needs. However, I appreciate the |
🤖 AutoRest automatic bundle size check 🤖Size increased by 0.00% (0 B) |
|
After speaking with TS team it seems like this is probably a reasonable design--we could consider factoring out Keeping that to a minimum is probably more desirable, and it sounds like we might be able to get a fix in the TS go-to-definition behavior that makes it so the candidate symbols are further constrained by a tagged union discriminator, which pretty much eliminates our remaining concerns about this approach. |
RikkiGibson
force-pushed
the
PolymorphicInterfaces
branch
from
9934039
to
5cadedb
Compare
|
Rebased to ensure compatibility with recent changes |
Fixes Azure/azure-sdk-for-node#2960
We're not 100% sure this is the approach we want, but we have played with a bunch of different possible representations and this so far seems to have the best editor experience.