Skip to content

Conversation

@mjashanks
Copy link

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)

@mjashanks
Copy link
Author

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?

@mgravell
Copy link
Member

mgravell commented Nov 3, 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

@mgravell
Copy link
Member

mgravell commented Nov 3, 2015

yeah, I think I made appveyor very angry... don't worry about that; appveyor doesn't cover contrib anyway, iirc!

@johandanforth
Copy link
Contributor

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?

@rhubley
Copy link

rhubley commented Nov 4, 2015

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)

@johandanforth
Copy link
Contributor

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?

@mjashanks
Copy link
Author

Happy to make the suggested changes if you agree its a feature that you need...

Use cases for this:
1.My legacy system uses column prefixes e.g. customerForename, customerSurname. This makes my POCOs really ugly

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.

@mjashanks
Copy link
Author

Pulled from master, and made the requested changes from code review...

@NickCraver
Copy link
Member

I'll be honest here - I don't think this is the right place for this feature. There are a few issues I see:

  • It's a new attribute - why are we adding another? This is what [DataMember] is for.
  • It adds complication to every method, as does every "serialization" change here - both the mapping and on the generation side.
  • I think there's a much better way.

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 DefaultTypeMap - for example one that uses DataMember as a column name source...or honestly maybe we do that in core.

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?

@NickCraver
Copy link
Member

Closing this one out for inactivity and the comments above - ping if you want to reopen discussion on it!

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.

5 participants