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

Always use TypeHandler for parameters if available #177

Closed
wants to merge 4 commits into from

Conversation

matwilko
Copy link
Contributor

Previously TypeHandlers could override the logic for built-in SQL data
types (int, byte, DateTime, etc.) when they were return values via
ITypeHandler.Parse, but ITypeHandler.SetValue was never called for these
types when used as parameters. Moved the typeHandlers.TryGetValue() call
up to make sure a type handler is always used if available.

Added tests to confirm standard behaviour is still working, and that ITypeHandler.SetValue is now called for built-in types.

matwilko and others added 4 commits September 22, 2014 05:23
Previously TypeHandlers could override the logic for built-in SQL data
types (int, byte, DateTime, etc.) when they were return values via
ITypeHandler.Parse, but ITypeHandler.SetValue was never called for these
types when used as parameters. Moved the typeHandlers.TryGetValue() call
up to make sure a type handler is always used if available.
Allow passing an IDictionary<string, object> as parameter object
Previously TypeHandlers could override the logic for built-in SQL data
types (int, byte, DateTime, etc.) when they were return values via
ITypeHandler.Parse, but ITypeHandler.SetValue was never called for these
types when used as parameters. Moved the typeHandlers.TryGetValue() call
up to make sure a type handler is always used if available.
@kartofeell
Copy link

I have applied changes from this PR on the latest master, and it works like a charm for me. TypeHandler.Parse and TypeHandler.SetValue are called as expected.

@kartofeell
Copy link

It would be great if this fix could get in. I guess there is not many conflicts to resolve, right?

@matwilko
Copy link
Contributor Author

I would imagine not if you were able to simply apply the changes to the current master, however even then it is still a matter of Marc's time, since he needs to verify the PR before merging.
Functionality within Dapper may well have changed around this in the meantime though, it has been 11 months after all!

@NickCraver
Copy link
Member

Do you mind rebasing this to current master so we can take a proper look and merge it in?

@matwilko
Copy link
Contributor Author

matwilko commented Sep 3, 2015

Sure, I'll have a look this evening.

@NickCraver
Copy link
Member

It looks like we can't actually pull this in due to comments mentioned on #339. It'll require some more digging to see if we can do this in a workable way - punting for the moment while we do some maintenance to make all future PRs easier though.

@nil4
Copy link

nil4 commented Jun 5, 2016

In #339 (comment):

Since we can't merge either - I'll consolidate this into #177 so we can track the issue in one place.

However, both issues are now closed despite not being resolved; @NickCraver consider reopening.

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

Successfully merging this pull request may close these issues.

6 participants