Skip to content

Infer literal/parameter's unicodeness from property#4937

Merged
smitpatel merged 1 commit intodevfrom
unicoded
Mar 31, 2016
Merged

Infer literal/parameter's unicodeness from property#4937
smitpatel merged 1 commit intodevfrom
unicoded

Conversation

@smitpatel
Copy link
Contributor

Part of #4686

private readonly ISqlGenerationHelper _sqlGenerationHelper;
private readonly IParameterNameGeneratorFactory _parameterNameGeneratorFactory;
private readonly IRelationalTypeMapper _relationalTypeMapper;
private static readonly bool _defaultUnicodeBehaviour = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Move static above instance fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and const?

@anpete
Copy link
Contributor

anpete commented Mar 31, 2016

LGTM, cc @ajcvickers for TypeMapper changes.

@ErikEJ
Copy link
Contributor

ErikEJ commented Mar 31, 2016

Providers beware?

@divega
Copy link
Contributor

divega commented Mar 31, 2016

@ErikEJ yes.

@divega
Copy link
Contributor

divega commented Mar 31, 2016

Are we setting the right unicodeness on parameters for update and delete statements for tables with non-Unicode string keys? I haven't tested it but I suspect performance of those could be affected by the same database limitation.

: base.FindMapping(clrType));
}

public override RelationalTypeMapping FindMapping(Type clrType, bool unicode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factor these two methods into a single method rather than copy/pasting all the logic.

@smitpatel smitpatel force-pushed the unicoded branch 2 times, most recently from d3cd900 to c4f2b6d Compare March 31, 2016 20:22
@smitpatel
Copy link
Contributor Author

@divega - This PR does not make any modification to update pipeline. Is there any issue tracking it?

@divega
Copy link
Contributor

divega commented Mar 31, 2016

@smitpatel there isn't a separate issue for updates. I can create one if you just want to get this in faster.

@smitpatel
Copy link
Contributor Author

This one is marked for RC2 milestone. Should get this in before the ask mode. 😄

@smitpatel
Copy link
Contributor Author

Updated with all feedback.

@ajcvickers
Copy link
Contributor

:shipit:

@smitpatel smitpatel force-pushed the unicoded branch 2 times, most recently from 17f6157 to d484f68 Compare March 31, 2016 21:30
@smitpatel smitpatel merged commit f416dd9 into dev Mar 31, 2016
@smitpatel smitpatel deleted the unicoded branch March 31, 2016 22:34
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