Description column (#33)

timabell / ef-enum-to-lookup

Generates lookup tables from enum references in Microsoft Entity Framework 6.1

https://www.nuget.org/packages/ef-enum-to-lookup

69 stars 29 forks source link

Description column (#33) #35

Open sveinhelge opened 9 years ago

sveinhelge commented 9 years ago

https://github.com/timabell/ef-enum-to-lookup/issues/33

This will be a breaking change in of terms usage of Description on Enum named constants.

Description will now be used as column and not to override name.

public enum Shape { Square, [System.ComponentModel.Description("Round it is!")] Round }

timabell commented 9 years ago

I haven't forgotten about this, will look when I get some spare time. Thanks for the PR!

timabell commented 9 years ago

Looks like your indents are spaces, but this project is using my personal preference of tabs. See https://github.com/timabell/ef-enum-to-lookup/blob/master/.editorconfig

timabell commented 9 years ago

You've done something slightly odd on the history of your branch (looking at the history in gitk). It diverges with two almost identical commits and then merges again. It would be better if your branch just had a single commit with your changes I think.

timabell commented 9 years ago

I've started a maintenance branch for v1 (called v1), and master is now for the v2 release.

timabell commented 9 years ago

I've created a temporary branch while I look closer at the code you've provided and try it out on my machine. https://github.com/timabell/ef-enum-to-lookup/tree/description-column - this fixes the whitespace issues.

timabell commented 9 years ago

So having had a closer look at the overall change I like it, and thanks for your work on this. Mostly my feedback is just trivia.

There's one bigger thing that's bugging me, which is that as far as I can tell it's no longer possible to override the strings that will go in the "Name" column, which is a feature I'd like to keep. I think you're right about making the description attribute control the description column instead of the name as it's more consistent, even though it's a fairly major change from v1. I think I'd like to see a solution to this problem before this gets merged in. My first thought is supporting a second attribute to control the name field; I'm not sure if there's an existing one that would be suitable, or whether we should create a new one, I'd probably want to do a bit of hunting in the microsoft APIs to see what's already out there.

sveinhelge commented 9 years ago

Could we use System.ComponentModel.DataAnnotations.DisplayAttribute?

Like this:

public enum Shape { Square,
[Display(Name = "Rounded", Description = "Round it is!")] Round }

timabell commented 9 years ago

I've had a read about it and that looks like a good choice to me.

timabell commented 9 years ago

I'm out of time/energy for the moment. It'd definitely be good to get this cleaned up and merged in.

Todo:

switch to displayattribute - make sure both display and description are covered properly in the unit tests
~~move typo fix to v1 branch (this is the wrong place for it)~~
put unit test in the right project
unit tests for merge of lookup rows
review updated test coverage
deal with project file noise x2 one way or the other
put the check around the db existing in a different commit with explanatory comment / commit message
check final diffs
cleanup commit history
merge into master

timabell commented 9 years ago

my description-column branch has been squashed & rebased onto the latest master, still more to do on it but that's now the best place to start

sveinhelge commented 9 years ago

I did some testing and in this branch. I used DisplayAttribute and added some more unit tests. Also used more SqlQuery in the tests.

https://github.com/sveinhelge/ef-enum-to-lookup/tree/description-column

We can probably used some of the code.

timabell commented 9 years ago

Thanks @sveinhelge

At a glance it looks promising, couple of bits of feedback:

How about EnumDisplayName instead of EnumNameValue
The lack of indentation in the generated sql is intentional, I want the output sql to look right even though it looks odd in the c#, as the sql is more visible to the users of the library
Your branch isn't based on my description-column branch at the moment, or even on master so I can't apply it as it stands. Make sure you have got the latest from my repo and take a look with gitk --all to see what I mean.

Thanks for your efforts on this, it is appreciated. I hope you don't mind me being picky, this is a showcase project for me, and the code in it needs to be something I'd be comfortable looking after in the longer term.

sveinhelge commented 9 years ago

EnumDisplayName is a good name.

This was a "offroad" test. So normally I would based it on master.

Picky makes things better. So no problem.