Adding mixed text and elements to collections using IList.Add. #103
Conversation
Types that don't have a `TypeConverter` should call `IList.Add`.
Ensure that `GetObject` is emitted when there is a collection that contains text, and use `IList.Add` to add items to collections, so that text can be added as a collection item.
grokys
changed the title
|
Changed this to be WIP as there's a few issues with it still. |
|
Hey @grokys, looking good, though I'd like to keep |
Failing reading a property after collection items.
And simplify `ReadCollectionItems` in the process.
We're testing `InlineCollection` and friends, lets not be shy about that. Move these tests to a new test class so they're a bit more manageable.
grokys
changed the title
|
Ok, @cwensley this is now ready for review I think. I hope I've got the |
| } | ||
| catch (TargetInvocationException e) | ||
| { | ||
| throw e.InnerException; |
There was a problem hiding this comment.
Hm, this would actually trash the original stack trace (where the crash actually happened). I think we should probably leave this try/catch out, unless you have a specific reason for this?
There was a problem hiding this comment.
I think the purpose of this is throw the original exception raised inside delegate. I don't see any reason why we can't unwrap delegate invocation from try block.
There was a problem hiding this comment.
Ohh... I see why it wrapped and re-throwed InnerException after remove this I have failed test: AddToCollectionTypeMismatch(). Assert.Throws<ArgumentException> (() => i.AddToCollection (l, "5")); We must leave this or we can rewrite test and scan ArgumentException in ``InnerException``` or something else? @cwensley what do you think about this?
There was a problem hiding this comment.
@cm4ker take a look at ExceptionDispatchInfo, it can be used to rethrow the inner exception without losing the stack trace. I've used it before here and it works well.
| @@ -171,7 +171,7 @@ static XmlReaderSettings CreateReaderSettings(XamlXmlReaderSettings settings, Co | |||
| CloseInput = closeInput ?? settings?.CloseInput ?? false, | |||
| IgnoreComments = true, | |||
| IgnoreProcessingInstructions = true, | |||
| IgnoreWhitespace = true, | |||
| //IgnoreWhitespace = true, | |||
There was a problem hiding this comment.
Is this test code? or is IgnoreWhitespace's default false?
There was a problem hiding this comment.
I think this is not a test code. When IgnoreWhitespace = false then inline tests go fault. Can I just remove commented line in this case?
|
@cwensley It's ready for next review round. |
|
@cm4ker looks beautiful! Thanks! |
The previous PR #102 fixed adding mixed text and elements to collections if a
TypeConverterwas available for the type, however System.Xaml doesn't require this - it will try to add inline text in an element to a collection usingIList.Add.This PR implements that behavior, but it also changes the behavior of Portable.Xaml in that it no longer tries to add the item using a typed
IList<T>.Addmethod and just goes straight forIList.Add. I'm not sure if that change in behavior is desired? Should we be trying the typedAddmethod first and falling back toIList.Add?I also suspect some of the logic around adding to collections could be tidied up here, but I didn't want to make that change in this PR.