search

sapiens / SqlFu

Fast and versatile .net core data mapper/micro-orm
Other
229 stars 50 forks source link

Support for storing SqlFu-specific attributes in metadata classes #27

Closed CaspianCanuck closed 10 years ago

CaspianCanuck commented 10 years ago

Yet another resubmission of https://github.com/sapiens/SqlFu/pull/26

Third time is the charm, hopefully! :)

sapiens commented 10 years ago

Are you using this feature? Either I don't know how to declare a buddy class or it works only for [Table]? I'd appreciate if you can provide me with an example

sapiens commented 10 years ago

OK, I got it, there were additional checks for custom attributes which needed to be changed.

CaspianCanuck commented 10 years ago

Like what additional checks? Did I miss anything in my code? I tested the enhanced GetSingleAttribute method with a unit test and it seemed to work fine.

On Wed, Nov 27, 2013 at 6:28 AM, Mihai Mogosanu notifications@github.comwrote:

OK, I got it, there were additional checks for custom attributes which needed to be changed.

— Reply to this email directly or view it on GitHubhttps://github.com/sapiens/SqlFu/pull/27#issuecomment-29377481 .

Tim

P.S. If you have trouble reaching me at tim@askerov.net tim@askerov.net please try taskerov@gmail.com taskerov@gmail.com instead.

sapiens commented 10 years ago

It worked only for 3 attributes: Table, InsertAsString and QueryOnly. The table def attributes were not covered :)

CaspianCanuck commented 10 years ago

Yes, I see what you mean! I missed a bunch of other places where other attributes are used in SchemaFromType.cs and AbstractMigrationTask.cs. Have you made the changes already or do you want me to make them and submit the files?

Also, in SchemaFromType.ProcessIndexes you use your GetCustomAttributes extension method rather than GetSingleAttribute, so I am going to have to modify that one as well.

On Wed, Nov 27, 2013 at 10:44 AM, Mihai Mogosanu notifications@github.comwrote:

It worked only for 3 attributes: Table, InsertAsString and QueryOnly. The table def attributes were not covered :)

— Reply to this email directly or view it on GitHubhttps://github.com/sapiens/SqlFu/pull/27#issuecomment-29394276 .

Tim

P.S. If you have trouble reaching me at tim@askerov.net tim@askerov.net please try taskerov@gmail.com taskerov@gmail.com instead.

sapiens commented 10 years ago

Lol, I missed the AbstractionMIgrationTask as well. But it doesn't require changes. Migration attributes are only for migration tasks, no model involved

CaspianCanuck commented 10 years ago

I see your changes now. But can't your GetSingleAttribute<>() simply return GetModelAttributes<>().SingleOrDefault() ? Then the real change will only be necessary in the GetModelAttribute method, not in both.

On Wed, Nov 27, 2013 at 11:28 AM, Mihai Mogosanu notifications@github.comwrote:

Lol, I missed the AbstractionMIgrationTask as well. I released today the version containing the changes. But probably I'll do another release to cover the migration tasks. It should be quick

— Reply to this email directly or view it on GitHubhttps://github.com/sapiens/SqlFu/pull/27#issuecomment-29398157 .

Tim

P.S. If you have trouble reaching me at tim@askerov.net tim@askerov.net please try taskerov@gmail.com taskerov@gmail.com instead.

CaspianCanuck commented 10 years ago

Another question: Do I understand it correctly that currently there's no way to override a property's column name? Looking at SchemaForType.ProcessColumn I can see that the column name is the same as PropertyInfo.Name. Is there a specific reason you chose not to allow column names to be overridden using your ColumnOptionsAttribute or even System.ComponentModel.DataAnnotations.ColumnAttribute?

On Wed, Nov 27, 2013 at 11:34 AM, Tim Askerov tim@askerov.net wrote:

I see your changes now. But can't your GetSingleAttribute<>() simply return GetModelAttributes<>().SingleOrDefault() ? Then the real change will only be necessary in the GetModelAttribute method, not in both.

On Wed, Nov 27, 2013 at 11:28 AM, Mihai Mogosanu <notifications@github.com

wrote:

Lol, I missed the AbstractionMIgrationTask as well. I released today the version containing the changes. But probably I'll do another release to cover the migration tasks. It should be quick

— Reply to this email directly or view it on GitHubhttps://github.com/sapiens/SqlFu/pull/27#issuecomment-29398157 .

Tim

P.S. If you have trouble reaching me at tim@askerov.net tim@askerov.net please try taskerov@gmail.com taskerov@gmail.com instead.

Tim

P.S. If you have trouble reaching me at tim@askerov.net tim@askerov.net please try taskerov@gmail.com taskerov@gmail.com instead.

sapiens commented 10 years ago

Well, I intended SqlFu to be only a data mapper on steroids, I don't want to evolve it in a real ORM. So, there are POCOs used to map query results and there are POCOs used to create tables in a quick and easy way(and which, being POCOs can be also used for querying). The point is they aren't really meant to model entities like an ORM does.

I have POCOs which represent tables and POCO which represent query results, sometimes they are the same, but at least the intention is to be considered distinct. So POCO_A representing table Users is not meant to be used as a business object or even as a view model, it's there only to 'be' a table. Sure, you can use it for everything, but I am a strong believer in decoupling and SoC and I don't want users to misuse it as they usually misuse an ORM.

My intention is that SqlFu to be a group of db access tools not a way to abstract a rdbms, to keep it simple. There are already too many ORMs or soon to be ORMs.

CaspianCanuck commented 10 years ago

Thank you for the informative answer! It makes perfect sense.

On Wed, Nov 27, 2013 at 1:25 PM, Mihai Mogosanu notifications@github.comwrote:

Well, I intended SqlFu to be only a data mapper on steroids, I don't want to evolve it in a real ORM. So, there are POCOs used to map query results and there are POCOs used to create tables in a quick and easy way(and which, being POCOs can be also used for querying). The point is they aren't really meant to model entities like an ORM does.

I have POCOs which represent tables and POCO which represent query results, sometimes they are the same, but at least the intention is to be considered distinct. So POCO_A representing table Users is not meant to be used as a business object or even as a view model, it's there only to 'be' a table. Sure, you can use it for everything, but I am a strong believer in decoupling and SoC and I don't want users to misuse it as they usually misuse an ORM.

My intention is that SqlFu to be a group of db access tools not a way to abstract a rdbms, to keep it simple. There are already too many ORMs or soon to be ORMs.

— Reply to this email directly or view it on GitHubhttps://github.com/sapiens/SqlFu/pull/27#issuecomment-29407931 .

Tim

P.S. If you have trouble reaching me at tim@askerov.net tim@askerov.net please try taskerov@gmail.com taskerov@gmail.com instead.