Description column (#33) #35

sveinhelge · 2015-07-31T12:33:16Z

#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
}

…enum-to-lookup into DescriptionColumn Conflicts: ExampleUsage/EnumExample.cs

timabell · 2015-08-05T15:12:18Z

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

timabell · 2015-08-05T21:03:36Z

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 · 2015-08-05T21:22:50Z

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 · 2015-08-05T21:38:01Z

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

timabell · 2015-08-05T21:41:33Z

EfEnumToLookupTests/EfEnumToLookupTests.csproj

@@ -100,6 +100,9 @@
      <Name>EfEnumToLookup</Name>
    </ProjectReference>
  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />


Could you tell me what this is? It's not something I recognise. Cheers

I found this https://connect.microsoft.com/VisualStudio/feedback/details/800245/vs2013rc-adds-to-vs2012-c-project-section-itemgroup but it's up to microsoft's usual standard of providing zero useful information.

I think VS 2015 is trying to be "helpful". I'm not sure why this was added.

Typical, lol. I'm still on 2013 for the moment. I'd take this out until we at least know what it does.

timabell · 2015-08-05T21:53:38Z

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 · 2015-08-05T22:00:36Z

EfEnumToLookup/LookupGenerator/SqlServerHandler.cs

@@ -111,12 +117,12 @@ private string PopulateLookup(LookupData lookup)
 	WHEN MATCHED AND src.Name <> dst.Name THEN
 		UPDATE SET Name = src.Name
 	WHEN NOT MATCHED THEN
-		INSERT (Id, Name)
-		VALUES (src.Id, src.Name)
+		INSERT (Id, Name, Description)


this won't pick up changes in description due to the "when matched" clause above

My bad. The "when matched" needs to be updated to catch description changes.

this indicates we could do with extending the test suite to cover add/remove/updating the lookup rows in the db as the model is changed.

plan: run apply, directly manipulate the db to add/edit/remove, assert changed (just to avoid false positive if this step silently failed), run apply again, assert that table was returned to original state

timabell · 2015-08-05T22:22:35Z

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 · 2015-08-05T23:30:15Z

Could we use System.ComponentModel.DataAnnotations.DisplayAttribute?

Like this:

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

timabell · 2015-08-08T21:01:33Z

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

"enum" could confuse people with c# enums pr #35

timabell · 2015-08-17T01:51:01Z

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 · 2015-08-18T06:39:51Z

ExampleUsage/EnumExample.cs

-	}
+
+        [Test]        
+        public void CheckIfDescriptionColumnsExists()


just noticed, this test is in the wrong project, this project is an example for users of the library of how it's intended to be used, not just generic unit test code, this needs to go in the EfEnumToLookupTests project

I agree.

TestConfiguration is an integration test and not an unit test. It might be good thing to separate unit test from integration tests a little more clearly I think.

You're right the test suite is a little inelegant at the moment and could do with tidying up and splitting out. I haven't got as far as thinking about the set of variations it needs to cover, it exists in its current form purely to enable me to test-drive the development to avoid regressions.

contribution from PR #35 - Svein Helge <svein.helge.grinden@gmail.com> fixed-up, squashed, rewritten by Tim Abell still stuff that needs fixing up in this commit - see pull request for details

timabell · 2015-08-18T08:16:21Z

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 · 2015-08-18T20:03:44Z

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 · 2015-08-18T23:02:07Z

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 · 2015-08-23T12:41:17Z

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.

sveinhelge added 5 commits July 31, 2015 13:38

New column with name Description added to the table.

72b693d

New column with name Description added to the table.

5f93beb

Merge branch 'DescriptionColumn' of https://github.com/sveinhelge/ef-…

abb1258

…enum-to-lookup into DescriptionColumn Conflicts: ExampleUsage/EnumExample.cs

Version bump v2.0.0

a58e5b7

Version bump v2.0.0-beta

249483a

timabell reviewed Aug 5, 2015
View reviewed changes

Added support for custom Schema.

e571410

timabell added a commit that referenced this pull request Aug 17, 2015

less ambiguous name for lookup row variable

9c7688f

"enum" could confuse people with c# enums pr #35

timabell reviewed Aug 18, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Description column (#33) #35

Description column (#33) #35

sveinhelge commented Jul 31, 2015

timabell commented Aug 5, 2015

timabell commented Aug 5, 2015

timabell commented Aug 5, 2015

timabell commented Aug 5, 2015

timabell Aug 5, 2015

sveinhelge Aug 5, 2015

timabell Aug 5, 2015

timabell commented Aug 5, 2015

timabell Aug 5, 2015

sveinhelge Aug 5, 2015

timabell Aug 17, 2015

timabell commented Aug 5, 2015

sveinhelge commented Aug 5, 2015

timabell commented Aug 8, 2015

timabell commented Aug 17, 2015

timabell Aug 18, 2015

sveinhelge Aug 23, 2015

timabell Aug 26, 2015

timabell commented Aug 18, 2015

sveinhelge commented Aug 18, 2015

timabell commented Aug 18, 2015

sveinhelge commented Aug 23, 2015

Description column (#33) #35

Are you sure you want to change the base?

Description column (#33) #35

Conversation

sveinhelge commented Jul 31, 2015

timabell commented Aug 5, 2015

timabell commented Aug 5, 2015

timabell commented Aug 5, 2015

timabell commented Aug 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabell commented Aug 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabell commented Aug 5, 2015

sveinhelge commented Aug 5, 2015

timabell commented Aug 8, 2015

timabell commented Aug 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabell commented Aug 18, 2015

sveinhelge commented Aug 18, 2015

timabell commented Aug 18, 2015

sveinhelge commented Aug 23, 2015