Feature/custom table name mapping #12
Conversation
|
Let me know if you're happy with this and then I'll do another commit with some documentation added to the readme. |
| /// <summary> | ||
| /// Helper for table related operations. | ||
| /// </summary> | ||
| public static class TableHelper |
There was a problem hiding this comment.
This class should probably be a service that's dependency injected with the rest of the dependencies. Something like:
public class NameService : INameService
{
public string GetTableName<TEntity>()
{
}
}
Then, this class would be dependency injected in the AddDapperGraphQL() extension method.
What do you think?
There was a problem hiding this comment.
Totally up to you. I just followed the pattern you'd used for the PropertyHelper, but I agree it'd be nice to have the flexibility to let others supply their own implementation if need be.
Should we leave the caching up to their implementation or have a base class that can/must be used that does it?
There was a problem hiding this comment.
I would change it to a service that's dependency injected. The ParameterHelper is different because it's more a caching interface between .NET and GraphQL -- it doesn't matter who uses it to benefit from the cache, whereas it may matter what database you're connecting to for the TableHelper to perform properly.
There was a problem hiding this comment.
I started playing around with this and the problem is all the static methods that create Insert/Query/Delete/Update contexts would need to accept an INameService because it can't be automatically resolved. Potentially this needs a bigger shift to use DI completely to new up these contexts. Have stopped for now as I'd like to hear your thoughts.
There was a problem hiding this comment.
I'll see if I can take a look over the weekend. Thanks Ben!
There was a problem hiding this comment.
Was just poking around Dapper.Contrib and I wonder if it's something we can depend on our take advantage of. They do some stuff around table name mapping.
There was a problem hiding this comment.
Looks like they have a default implementation and then a delegate you can supply to override the default if needed. We could look at an approach like that instead of dependency injection potentially.
There was a problem hiding this comment.
Hello,
Why is it still not merged?
I also want to have an ability to specify table name because the default table name is not always suitable.
There was a problem hiding this comment.
Need to change code to use Dapper.Contrib functionality?
|
TODO: look at whether we could/should use this: https://twitter.com/ben_a_adams/status/988951647639531522?s=19 |
|
Hey @Alexcei88, I started on this and then hit a bit of a roadblock with the changes as @dougrday wanted the naming service to be DI-able but it didn't really fit in well with the existing codebase that wasn't super DI-friendly. I then thought about Dapper.Contrib and what they offer as a solution but didn't go much further than that. If @dougrday is keen to pick this back up and have a look perhaps he can finish it off for you. Let's see what he says. |
|
I'm moving to a new position at a new company, stay tuned, I should have this reviewed/merged soon once that settles down a bit. |
|
Gonna close this as it's pretty stale. Will keep my fork around for a while longer. |
Addresses: #9