-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix for XmlDeserializer issue not handling lowercase + dash root elements in some cases #279
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
Conversation
…dashed" version for element names. Issue was reported here: http://groups.google.com/group/restsharp/browse_thread/thread/c70c39b47d10002d
restsharp#269 The checkin consist of a new test: Can_Deserialize_Root_Elements_Without_Matching_Case_And_Dashes Which will deserialze a structure like this: <incoming-invoices> <incoming-invoice> ... <incoming-invoice> </incoming-invoices> To a List<IncomingInvoice>
if existing tests pass too, this is good to pull |
Ok thanks, they pass fine... |
Hi John, Do you have a time frame when this can be merged, or is there anything I can do? |
Bump - would like to see this merged. |
Yeah, think I just submitted a similar/same pull request. I'll withdraw mine, would love to see this get merged in. |
By the way, it looks like xml attributes have the same sort of bug which isn't addressed by this pull request. Seems like trying all the different convention mappings is something that could be extracted into a reusable piece of code, right? |
@SaintGimp Sounds like a good idea... @johnsheehan I finally learned about rebasing (;0) so I if it makes it easier for you I could probably create a cleaner pull request? |
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.
Instead of removing this existing test for LowercaseUnderscores, can we supplement the lowercase underscores test with an additional test for dashes? Doing so will help us ensure both deserialization approaches stay working after future changes.
Ok, merged, and I restored the one existing test to make sure we cover both cases, and did some minor whitespace fixes. Thanks for the contribution! |
Thanks! Any idea when this is released to a nuget package, or is there feed somewhere? |
@ayoung released a new build 103.2 today to nuget.org. You should be able to update and have the change. |
Got it, thanks! |
Added a fix and a unit test for the issue: #269
The checkin consist of a new test:
Can_Deserialize_Root_Elements_Without_Matching_Case_And_Dashes
Which will deserialze a structure like this:
To a List
<IncomingInvoice>