New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support open generic metatypes #857
base: main
Are you sure you want to change the base?
Conversation
src/protobuf-net/Meta/MetaType.cs
Outdated
var metaType = new MetaType(model, type, factory); | ||
metaType.SetSurrogate(surrogateType.GetGenericTypeDefinition()); | ||
metaType._fields = _fields; | ||
return metaType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this part will need further refinement. I'm assigning _fields
versus duplicating the field data as it should be an exact match of data - I can't think of a way this wouldn't work. Happy to instead iterate over the data and manually call AddField
etc.
Also there may be other important properties to set from one MetaType
instance to the other here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code to include additional properties on MetaType
that seemed relevant but I'd need your expertise to confirm my approach and whether there are any other considerations I should be aware of.
var genericMetaType = (MetaType)types[key]; | ||
if (genericMetaType.Pending) | ||
{ | ||
WaitOnLock(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kinda winging this part here based on existing code further up in the method.
if (metaType is null) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This additional check seemed like the best solution to avoid triggering the next block if a metatype was generated in the new code I wrote. In my head it was either this or a goto
statement.
Not sure why the build failed - as far as I can tell, all the tests passed on AppVeyor 🤷♂️ |
11800ab
to
1652f18
Compare
Hey @mgravell, if you have the time, I'd love to get your feedback on the fix - in particular, my changes in If you can't look at it at the moment because your plate is full, I totally understand! |
Fixes #802
Currently you can't write something like this:
The problem is that the "type" is typically matched exactly however by writing such code, I'm intending it match all types that use that generic definition.
The fix is accomplished by looking up by the generic definition if it has failed to find an exact match or a contract attribute specified. If found, it generates a new metatype entry, using the fields and surrogate from the generic definition's metatype entry.