Combine ExpandoObjectAdapter and DictionaryAdapter #106
Conversation
| } | ||
| } | ||
| } | ||
| return newValue; | ||
| } | ||
|
|
||
| private static T TryParse<T>(string value) |
There was a problem hiding this comment.
@rynowak Can we talk about what's the best way to either combine the two TryParse methods or if there is a better way to convert keys and values to TKey and TValue?
| dictionary[key] = ConvertValue(dictionary, key, value); | ||
| var convertedKey = TryParse<TKey>(key); | ||
| var convertedValue = TryParse<TValue>(value); | ||
| dictionary[convertedKey] = ConvertValue(dictionary, convertedKey, convertedValue); |
There was a problem hiding this comment.
Instead of TryParse methods, could you use ConversionResultProvider.ConvertTo for TKey (this could also solve the issue of non-string keys in dictionary) and use ConvertValue for TValue (you need not use TryParse for value as you are anyway going to convert the value)?
|
|
||
| // As per JsonPatch spec, if a key already exists, adding should replace the existing value | ||
| dictionary[key] = ConvertValue(dictionary, key, value); | ||
| var convertedKey = (TKey)ConversionResultProvider.ConvertTo(key, typeof(TKey)).ConvertedInstance; |
There was a problem hiding this comment.
What if this conversion fails? Casting a null value to int for instance will throw an exception
| { | ||
| return new DictionaryAdapter(); | ||
| var type = typeof(DictionaryAdapter<,>).MakeGenericType(jsonDictionaryContract.DictionaryKeyType, jsonDictionaryContract.DictionaryValueType); | ||
| return (IAdapter)Activator.CreateInstance(type); | ||
| } |
There was a problem hiding this comment.
We may still need to handle the case where an object is an IDictionary but not an IDictionary<>
There was a problem hiding this comment.
We've decided not to do anything about this.
| @@ -18,11 +17,11 @@ public void Add_KeyWhichAlreadyExists_ReplacesExistingValue() | |||
| var nameKey = "Name"; | |||
| var dictionary = new Dictionary<string, object>(StringComparer.Ordinal); | |||
| dictionary[nameKey] = "Mike"; | |||
| var dictionaryAdapter = new DictionaryAdapter(); | |||
| var resolver = new Mock<IContractResolver>(MockBehavior.Strict); | |||
| var dictionaryAdapter = new DictionaryAdapter<string, object>(); | |||
There was a problem hiding this comment.
We need some tests for things besides <string, object>
There was a problem hiding this comment.
Working on adding these
| @@ -3,7 +3,6 @@ | |||
|
|
|||
| using System; | |||
| using System.Collections.Generic; | |||
There was a problem hiding this comment.
Do the expando object tests need an update?
There was a problem hiding this comment.
There are no unit tests for ExpandoObjectAdapter. Only integration tests, which pass using the new adapter
|
@rynowak Changed key and value conversion code |
|
|
||
| // As per JsonPatch spec, if a key already exists, adding should replace the existing value | ||
| dictionary[key] = ConvertValue(dictionary, key, value); | ||
| var canConvertKey = TryConvertKey(key, out var convertedKey, out errorMessage); | ||
| if (!canConvertKey) |
There was a problem hiding this comment.
You can just merge this into the if if you want it to be more terse.
| } | ||
| else | ||
| { | ||
| errorMessage = Resources.FormatInvalidValueForProperty(value); |
There was a problem hiding this comment.
This is a good message The value '{0}' is invalid for target location.
| } | ||
| else | ||
| { | ||
| errorMessage = Resources.FormatInvalidValueForPath(key); |
There was a problem hiding this comment.
I don't really like this error message since we're just looking at the 'key' not a whole path.
Can we say something like:
The provided path segment '{0}' cannot be converted to the target type.
|
This is looking pretty good now. Just waiting on a new error message and some tests. |
jbagga
changed the title
|
@rynowak Updated with tests |
Refactoring some code to combine the two (similar) adapters, ensuring both types use the
JsonDictionaryContractand resolve the dictionary keys.May need more tests. This is just to get feedback on some generic type conversion. See my comment below.
cc @rynowak