-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Added Column attribute - to alias column names #376
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
Conversation
|
Error from AppVeyor: "error : The Dnx Runtime package needs to be installed" ... Is there anything i can do about this? (using VS 2013 Express, which cannot open the DNX solution). Do i need to go get VS Community 2015? |
|
Excellent question; that is probably my fault - I've been iterating the DNX builds, updating to beta-7 (and today: beta-8); I don't know a lot about AppVeyor, but will investigate |
|
yeah, I think I made appveyor very angry... don't worry about that; appveyor doesn't cover contrib anyway, iirc! |
|
I'm not sure if this attribute is really needed. If column names don't match the properties of the class, you can simply change the property name, can't you? |
|
If column attribute is added, it should be similar to System.ComponentModel.DataAnnotations.Schema.ColumnAttribute and the code should handle both Column attributes. This is done now with System.ComponentModel.DataAnnotations.Schema.TableAttribute and the Dapper.Contrib table attribute as seen in method private static string GetTableName(Type type) |
|
Agree @rhubley , but I'm also thinking it adds quite a few more lines of code to all extension methods which shouldn't make much of an impact on speed when it's cached, but... do we need it? |
|
Happy to make the suggested changes if you agree its a feature that you need... Use cases for this: 2.There is the odd unfortunate case where a column has been reused, or evolved to take a different meaning. Naughty naughty, but it happens. Alternative Solution (that i'd be happy to implement): Introduce an interface 'IColumnNameStrategy', which could be implemented by the user, and added as a configuration option. Thus giving full control to the user over how columns are named. Defaulting to the current 'Strategy' of course, where ColumnName = PropertyName. |
|
Pulled from master, and made the requested changes from code review... |
|
I'll be honest here - I don't think this is the right place for this feature. There are a few issues I see:
I have no problem with the automatic mapping to the right columns and doing so based on conventions that you decide. An attribute everywhere is one way of doing this - but one of many. I'm seeing a common theme across about 20 issues here that simply scream: "Make the Type Mapper more pluggable". I think the mapper is the correct place for this - it decides what column maps back - why can't it also handle the other direction? If you it was pluggable, you could use any convention or source you wanted. With this, Contrib could simply have additional type mappers that extend the This may not be a trivial thing because of what speed is offered, but since we cache the result there's a chance we can do this right. I possibly see the key thing abstracted out in a similar fashion to repeat as little as possible across all extensions. @mgravell and everyone, thoughts? |
|
Closing this one out for inactivity and the comments above - ping if you want to reopen discussion on it! |
In Dapper.Contrib, I added an attribute 'ColumnAttribute'. This will allow us to name properties differently to how the corresponding column is named
e.g.
[Column("ResultName")]
public string Name { get; set; }
Added for all Contrib methods - Get, GetAll, Insert, Update, Delete, DeleteAll (and their Async conterparts)