Skip to content
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

Fix #253, use user defined TypeHandlers over internal implementations. #258

Closed
wants to merge 1 commit into from

Conversation

adminnz
Copy link

@adminnz adminnz commented Mar 24, 2015

basically SqlMapper was using its own internal IEnumerable type handling even if the user has defined their own type handler for IEnumerable types (e.g. List).

I've switched the order so it will check if the user has defined a type handler for this, and then use the IEnumerable implementation.
Also I added two tests, to DapperTests NET40 with a very simple List TypeHandler implementation that will read Strings into a List by splitting the string comma. And also will use join a list by comma and set a parameter from a List.

@espenekvang
Copy link

Any indications on when this pullrequest can be merged in? We have implemented a custom typehandler but SetValue is never called.

@jhovgaard
Copy link

Spent time "bug fixing" this too. +1 for merge.

@RufusJWB
Copy link

I have the same problem and it would be great, if it could be fixed.

@klesh
Copy link

klesh commented Jun 19, 2015

Support +1
It would be great to fix this problem.

@johandanforth
Copy link
Contributor

I merged the fix and the tests provided by @adminnz . Would you all please test things a bit and give feedback before I update Nuget as well? I normally only work with the Contrib parts but this one seems pretty obvious to merge in so...Thanks for your patience. ping @espenekvang @jhovgaard @RufusJWB @klesh

@klesh
Copy link

klesh commented Jun 20, 2015

Hi, Johan, thanks for your help.
I checked the test case and ran it, everything seems alright...
All two test cases are passed. @johandanforth

@johandanforth
Copy link
Contributor

Great. I'm closing this one then.

@RufusJWB
Copy link

I couldn't help testing, but I highly appreciate your quick response! Thank you very much!

@jhovgaard
Copy link

Thanks for the merge! Works here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants