Refactor lookup of dictionary key converters #48232
Conversation
|
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsRemoved the separate cached dictionary to contain the converters that support number handling and replaced with a virtual method This improves maintainbility especially for non-trivial converters like Enum and for other work that I'm prototyping. In the future, this would also be useful if we want to enable custom converters to add their own number handling.
|
|
@jozkee after thinking about this more, this could break some edge cases where a custom primitive (Int32 etc) converter registered with the options won't work with quoted numbers since that support is based on internal converter methods. Was this a reason for the current implementation? In either case, this edge case would likely cause issues anyway since their custom logic wouldn't be applied to quoted numbers. One factor I didn't mention earlier is that we need a fast way to convert a primitive to a string for the writable DOM work. It is not performant to new up a writer, get a pooled buffer, possibly transcode UTF8 to string, etc. just to convert a primitive to a string. So if we decide not to take this PR I can close this PR and address the refactoring with the writable DOM work instead including making the internal methods public. |
@steveharter if this PR is changing how we get the converter for the dictionary key by not only looking at the built-in converters but also at custom ones, then yes, that would be breaking since Read/WriteWithQuotes methods are not yet exposed. I think your approach could be possible if #46520 was addressed. |
OK closing this. We should add a test that uses a custom converter for a number type + use the quoted number feature. I may create a new PR that addresses other items here including the usage of ConcurrentDictionary here (standard Dictionary is fine since there are no threading issues) and also remove the double instances of the built-in converters in the two dictionaries. |
ghost
locked as resolved and limited conversation to collaborators
3 participants
Removed the separate cached dictionary to contain the converters that support number handling and replaced with a virtual method
SupportsQuotedNumbersthat returns true for the appropriate built-in converters.This improves maintainbility especially for non-trivial converters like Enum and for other work that I'm prototyping. In the future, this would also be useful if we want to enable custom converters to add their own number handling.