-
Notifications
You must be signed in to change notification settings - Fork 127
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
Enhancement: Use lambda to define property handlers within DTO constructor. #421
Comments
Historically, RepoDb understand the same base class named BaseEntity, but I removed it to clear some ground working before the development. Anyway, with regards to your classes, please allow me to review your changes and let us discuss if that is feasible for PR. I am thinking that once @guy-lud issue a PR, such request will be covered as well. Not to mention, in the latest Beta release, the classes named PropertyHandlerMapper and PropertyHandlerCache is provided to cater the expression-based property handling mapping. I am dreaming of having these: var mapper = new ClassMap<Customer> // or FluentMap<Customer>
.Name("[dbo].[TableName]")
.Primary(e => e.Id)
.Identity(e => e.Id)
.TypeMap(e => e.DateOfBirth, DbType.DateTime2)
.ProperHandler<AddressPropertyHandler>(e => e.Address); The underlying calls would simply call the existing mappers. public class ClassMap<TEntity> where TEntity : class
{
public ClassMap<TEntity> ProperyHandler<TPropertyHandler>(Expression<Func<TEntity, object>> expression)
{
PropertyHandlerMapper.Add<TEntity, TPropertyHandler>(expression);
return this;
}
...
} TBH, the changes would be small and less impact as new class will be introduced that calls the mappers. The full control would be 100% on the the PR issuer side. Of course, the Unit and Integration Tests must be written, and I can help here. In the operational POV and maintainability, it is easy as we are only looking at the Cachers and Mappers. Are you interested on PR? |
EDIT: I figure out how to create a pull request from another fork. Here's the link ... Cheers. |
I wonder why I could not see the PR directly to RepoDb master branch. |
There you go, I can now see it. Please allow me to review it. |
Thanks for taking the time to assess. RepoDB is a looking really good. I love what you did with your use of Linq Expressions. Cheers. |
@cwaldron - I liked that you dive down in the core compiler implementation of RepoDb (the FunctionFactory.cs) - that's a bit difficult. Thanks for that and cheers! Anyway, TBH, your changes seems to be a lot and it is a bit risky to do some acceptance of PR as big as this. Also, some of it is seems against to architectural decisions made historically. I would be very happy if you had contacted me if you had plan to do a PR like this so we had collaborate specific area to be changed. Apology but I think I am not approving these PR. On the other hand, I liked to merged some of your extension methods. Also, I am opening the implicit class/fluent mapping to the public, and if you try looking at it, it seems to be solving your problem as well - with much more simplier and better approach consolidated to a single class. You are free to take it if you would like, we can just notify @guy-lud. I hope this would be okay to you. |
@mikependon I really appreciated the feedback. It was one of those "what-if" things that I wanted to try out before contacting you. I wouldn't have made sense if I didn't have a working version. I didn't expect you to accept the PR but just to take a look at it and see if the approach would be useful. It really didn't take long to put together and I really marvel at your use of Linq Expression. There really is not much thorough documentation on using Linq Expression and I learned a lot by going through your code. After reading @guy-lud request, I realized that implementing BaseEntity class , despite being optional, is problematic because it makes the DTO dependent on the ORM. So I clearly understand your decision to reject the PR but it was fun anyway. Cheers, |
This request of yours can be addressed by FluentMapper
.Entity<Customer>() // Which Entity Model?
.Table("[sales].[Customer]") // Table/View Mapping
.Primary(e => e.Id) // Primary Mapping
.Identity(e => e.Id) // Identity Mapping
.Column(e => e.Id, "CustomerId") // Column Mapping
.Column(e => e.Name, "CustomerName") // Column Mapping
.DbType(e => e.DateOfBirth, DbType.DateTime2) // DbType Mapping
.PropertyHandler<CustomerAddressPropertyHandler>(e => e.Address); // PropertyHandler Mapping The feature is available at RepoDb (v1.11.0). > Install-Package RepoDb -version 1.11.0 Please visit the Implicit Mapping to learn more. |
I agree with request #415 because as @guy-lud mention, attributes on DTOs are extremely awkward. I opened this as a separate issue because I think this may not work within the existing ClassMap scheme. I want to define are property handlers within the DTO constructor. This makes it easier to organize and keep the logic tied to the DTO.
I would like to define the property handlers like this.
This modification fixed the three unit tests that were failing:
TestSqlConnectionCrudConversionFromStringToDateTime2
TestSqlConnectionCrudConversionFromStringToDateTime
TestSqlConnectionCrudConversionFromStringToDate
I modified the function factories (FunctionFactory.cs) to recognize BaseEntity as well as adding two helper classes to enable calling and caching these converters: EntityPropertyConverter, EntityPropertyMap
The current manner by which the property handlers are defined is quite awkward because you have to create a separate disjoined class for the handler operators (get, set). This way the handlers are defined as lambda directly in the class.
I think this could be added as part of the @guy-lud ClassMap request.
The text was updated successfully, but these errors were encountered: