DictionarySource can evaluate IReadOnlyDictionary<TKey,TValue>

[axunonb]

Closes #349

[codecov]

Codecov Report

Patch coverage: 97% and no project coverage change.

Comparison is base (6a395ec) 96% compared to head (0fbb00e) 96%.

❗ Current head 0fbb00e differs from pull request most recent head bd39877. Consider uploading reports for the commit bd39877 to get more accurate results

Additional details and impacted files

@@         Coverage Diff         @@
##           main   #353   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        91     91           
  Lines      3198   3230   +32     
===================================
+ Hits       3084   3115   +31     
- Misses      114    115    +1     

Files Changed	Coverage Δ
src/SmartFormat/Extensions/DictionarySource.cs	98% <97%> (-2%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[axunonb]

Sonar Cloud Quality Gate

[axunonb]

@karljj1 From a performance perspective IReadOnlyDictionary should go into into its own ReadOnlyDictionarySource that is not loaded by default, shouldn't it? Other thoughts?

[karljj1]

@karljj1 From a performance perspective IReadOnlyDictionary should go into into its own ReadOnlyDictionarySource that is not loaded by default, shouldn't it? Other thoughts?

It looks like this new code will have an impact on performance so maybe it should be moved into its own source. It would be nice if it was part of the DictionarySource but it looks like dealing with IReadOnlyDictionary is not simple.

[axunonb]

@karljj1 Thanks for the comment. I fully agree.
I wonder whether this "full" implementation is kind of an overkill. The much more simple alternative would be to add

if (current is IReadOnlyDictionary<string, object?> reDict)
{
    if (!reDict.TryGetValue(selector, out var val)) return false;

    selectorInfo.Result = val;
    return true;
}

below the check for IDictionary<string, object?>. The same limitations mentioned there would apply for IReadOnlyDictionary<string, object?>. I guess, this what @dombrovsky had in mind, too (see #342).
Does this sound like a good solution to you?

[karljj1]

@karljj1 Thanks for the comment. I fully agree. I wonder whether this "full" implementation is kind of an overkill. The much more simple alternative would be to add

if (current is IReadOnlyDictionary<string, object?> reDict)
{
    if (!reDict.TryGetValue(selector, out var val)) return false;

    selectorInfo.Result = val;
    return true;
}

below the check for IDictionary<string, object?>. The same limitations mentioned there would apply for IReadOnlyDictionary<string, object?>. I guess, this what @dombrovsky had in mind, too (see #342). Does this sound like a good solution to you?

It feels like trying to handle an edge case here, Supporting IReadOnlyDictionary<string, object?> should be fine although we may also want to support IReadOnlyDictionary<string, object> as well. If a user does want to use a read only dictionary then there's a chance that it won't be a <string,object> so I wonder if adding support for just that type is worth it. If we do support it then it's probably worth supporting all types in which case I would say make it a separate ReadonlyDictionarySource so that users can opt-in to the performance hit.

[axunonb]

@karljj1 Latest update - I agree to make it right or leave it:

• DictionarySource has property IsIReadOnlyDictionaryEnabled which is false by default. (This is to avoid a separate ReadonlyDictionarySource.)
• Add IDictionary<Type, (PropertyInfo, PropertyInfo)?> RoDictionaryTypeCache to cache types.
• Remove Linq statements
• Modified unit tests, coverage 91% - Sonar Cloud Quality Gate

[karljj1]

I like it. Much better to keep it optional :)