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 convert error for type level with propertyHandler #680

Merged
merged 3 commits into from Dec 27, 2020

Conversation

fredliex
Copy link
Contributor

fix #646 .

Please let me know if there is anything not considered.
I will try to fix it again.

thank

@mikependon
Copy link
Owner

Hi, can you provide more context on this PR? Let me understand your problem and the solution you introduced on the 3 files. Just a high-level information of the problem and solution would help a lot. Though, I will look at this PR soon.

Looking forward to hearing from you.

@fredliex
Copy link
Contributor Author

I want to use PropertyHandler to map Enum to any database type, which may be varchar, int, float, decimal... etc. of specific values.

If Enum is in the model, the original operation is expected.

public class SomeModel {
   public SomeEnum Prop1 { get; set; }  // mapping to decimal 100
   ......
}
connection.ExecuteQuery<SomeModel>("select convert(decimal, 100) as Prop1");

However, if you directly correspond to the type, there will be a transformation error. The reason is that the processing of the PropertyHandler will be skipped in some cases.

connection.ExecuteQuery<SomeEnum>("select convert(decimal, 100)");

The main correction of this PR is that when the type corresponds, the conversion type can be determined according to the type of the PropertyHandler to avoid conversion errors.

in GetClassPropertyParameterInfoIsDbNullTrueValueExpression
mainly changed is to obtain the propertyHandler based on the classProperty or entity type. If the propertyHandler exists, the target type of convertion is determined based on the propertyHandler.

in GetPlainTypeToDbParametersCompiledFunction
mainly changed is to determine if a propertyHandler exists, then the value of DbParameter.DbType is determined according to the propertyHandler.

@mikependon mikependon self-assigned this Dec 23, 2020
@mikependon
Copy link
Owner

This is just for me ~ I saw some out/ref in the code. Though, it was not a part of our coding standards yet, but I am trying to remove it from the way we do things in RepoDB. We have few of this historically, which I already started refactoring.

This is to avoid a back-door referencing of the variables, in which could add more complexities on the code and the way we do the debugging. If you have a chance, maybe try to refactor the code a little to remove the out/ref keywords.

Also, please give me a little time to review this one, maybe after the Christmas Holiday. :) Thanks for this PR.

@fredliex
Copy link
Contributor Author

I have considered the suitability of out parameter, but in the end, in order to avoid repeated get of the Set return type of the PropertyHandler, and to avoid a large amount of code modification, the out parameter is used first after the choice.

Or can it return ValueTuple?
If ValueTuple can be used, I can use ValueTuple instead of out parameter.

@mikependon
Copy link
Owner

mikependon commented Dec 24, 2020

This is the reason these objects class, MemberBinding class and FieldDirection exists.

Would you be able to place this one on a branch?

@mikependon mikependon closed this Dec 24, 2020
@mikependon mikependon reopened this Dec 24, 2020
@mikependon
Copy link
Owner

Fat finger, I am using my phone and accidentally close it. Reopening

@fredliex
Copy link
Contributor Author

What if it looks like this?

internal static Expression ConvertExpressionToPropertyHandlerSetExpression(Expression expression,
    ClassProperty classProperty,
    Type targetType)
    => ConvertExpressionToPropertyHandlerSetExpressionCore(expression, classProperty, targetType).expression;

internal static (Expression expression, Type handlerSetType) ConvertExpressionToPropertyHandlerSetExpressionCore(Expression expression,
    ClassProperty classProperty,
    Type targetType)
{ 
    return ......
}

This can omit the definition of temporary classes.

@mikependon
Copy link
Owner

It is ok to use Tuple, but I would say, just leave it for now and let me do the thorough review after the holidays, so I can provide more better insights. What I can say, the method name itself should do what it is meant to do. Definitely, naming should be Tuple and not Expression then :)

Btw, can you create a branch for this? Also, is this something you are needing very soon?

@fredliex
Copy link
Contributor Author

Sorry, I don't quite understand what you mean.
Does "create branch" mean in my fork repository?

There is no time pressure for nuget push.
But I hope to merge to master as soon as possible, so that my subsequent tests can continue.

thank

@mikependon
Copy link
Owner

@fredliex - anyway, you can just ignore my request as I can see your branch Fix/Enum_PropertyHandler under your own account, I can always work there.

Just in case you want to fix something in the near future, you can now create a branch directly to the main line as I just made you a collaborator.

@fredliex
Copy link
Contributor Author

I just tried to create a branch in your repository, but I still don't seem to have permission.

I have changed the out parameter to return tuple in Fix/Enum_PropertyHandler branch on fredliex/RepoDB repository.

thank.

@fredliex
Copy link
Contributor Author

Sorry, I didn't notice the collaborationator invitation letter.
I will try to branch here later.

@fredliex
Copy link
Contributor Author

ok, the branch fix_Enum_PropertyHandler has been established here.

Use the method of returning Tuple.

thank

@mikependon
Copy link
Owner

@fredliex - after the review, I would say that this is a very nice PR. Many thanks for this! I am currently running all our Integration Tests (cross data providers), hopeful to have all of them green. Cross fingers!

@mikependon mikependon merged commit c5ea48a into mikependon:master Dec 27, 2020
@mikependon
Copy link
Owner

Thanks again for pushing this @fredliex !

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.

Enhancement: can't custom db string -> enum property by IPropertyHandler<string, TEnum>
2 participants