Description column (#33)

[sveinhelge]

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]

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

[timabell]

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]

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]

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

[timabell]

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]

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]

Could we use System.ComponentModel.DataAnnotations.DisplayAttribute?

Like this:

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

[timabell]

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

[timabell]

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]

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]

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]

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]

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.