-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix deserialization failure message of combined types #8558
Conversation
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.
So, for all of these except FontWeight, the boolean is not something I'd like to recommend to people. It's something we did for convenience, or something we did for compatibility (closeOnExit), but never recommended. I'd be more inclined to leave those ones alone.
For FontWeight, though. We're only adding the exception handling so that we can call our local TypeDescription
, right?
It seems like FlagMapper and EnumMapper should just use TBase::TypeDescription
. That will let them automatically get the derived type's description with automatic rollup to the parent's implementation. That way, you don't need to catch/rethrow/etc. 😄
Actually, I wanted to make TypeDescription |
I'd rather we not do that -- right now it all compiles down to free immediate dispatch without vtable lookups... and we're not particularly helped by making it virtual (since there's workarounds here.) The converter type never changes at runtime, which is the big thing |
Yeah. Makes sense. That is why I avoided it in the first place. 😊 |
@DHowett - any reason TypeDescription is not static? I mean I can always do something like:
or to convert this method to static. |
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.
Love this. Yeah, static thing is weird. I'm interested in cleaning up JsonUtils a little in the future, too. Got a couple todo list items in there 😄
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.
Alright, seems cool to me. Thanks!
I'm gonna tag in @DHowett to massage the commit message and make sure that the build doesn't break on merging |
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.
Thanks!
🎉 Handy links: |
PR Checklist
fontWeight
is rejected by the profile parser #7690Validation Steps Performed
Manual tests