Skip to content

Add inference-based overload to ForNpgsqlHasEnum - #621

Merged
austindrenski merged 6 commits into
npgsql:devfrom
austindrenski:enum-inference
Sep 1, 2018
Merged

Add inference-based overload to ForNpgsqlHasEnum#621
austindrenski merged 6 commits into
npgsql:devfrom
austindrenski:enum-inference

Conversation

@austindrenski

@austindrenskiaustindrenski commented Aug 30, 2018

Copy link
Copy Markdown
Contributor

Closes: #620

  • PgNameAttribute is respected for the type and members.
  • Default translator from NpgsqlConnection.GlobalTypeMapper.DefaultNameTranslator

Related

#605

- `PgNameAttribute` is respected for the type and members.
@austindrenskiaustindrenski added this to the 2.2.0 milestone Aug 30, 2018
@austindrenskiaustindrenski self-assigned this Aug 30, 2018
@austindrenskiaustindrenski changed the title Add overload to infer from CLR enum to scaffoldAdd inference-based overload to ForNpgsqlHasEnumAug 30, 2018
Comment threadsrc/EFCore.PG/Extensions/NpgsqlModelBuilderExtensions.cs Outdated
Comment threadsrc/EFCore.PG/Extensions/NpgsqlModelBuilderExtensions.cs
Comment threadsrc/EFCore.PG/Extensions/NpgsqlModelBuilderExtensions.cs Outdated
@austindrenski

Copy link
Copy Markdown
ContributorAuthor

@YohDeadfall Updated per your review.

@austindrenski

Copy link
Copy Markdown
ContributorAuthor

Updated to Npgsql 4.1.0-ci.1270 to use the default name translator from npgsql/npgsql#2137.

roji
roji approved these changes Sep 1, 2018

@rojiroji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea.

Maybe update the doc enum samples to use this? After it's all it should the preferred method to define an enum assuming you have a CLR one...

One more point is to wait for #2139 to be merged and take a dependency on the appropriate 2.1.x nuget.

@austindrenski

austindrenski commented Sep 1, 2018

Copy link
Copy Markdown
ContributorAuthor

@roji I'm having some (user-)error building against the new Npgsql 4.0.3-ci.1272+sha.b3fc7da6f. Any idea what's happening?

Scratch that....looks like it fixed itself?

@austindrenski

Copy link
Copy Markdown
ContributorAuthor

@roji I've updated the docs to just show the simplest overload.

I'm now thinking about whether the old overloads should be obsoleted:

  • Scaffolding a PostgreSQL enum without a CLR enum is a strange operation.
  • Name alterations can be handled either with the new overload or via PgNameAttribute.

Thoughts?

@austindrenski

Copy link
Copy Markdown
ContributorAuthor

Going to merge this now. If we decide to obsolete the preexisting overloads, I'll submit that as a separate PR.

@austindrenski
austindrenski merged commit a9048e6 into npgsql:devSep 1, 2018
@austindrenski
austindrenski deleted the enum-inference branch September 1, 2018 22:45
@roji

roji commented Sep 4, 2018

Copy link
Copy Markdown
Member

Scaffolding a PostgreSQL enum without a CLR enum is a strange operation.

It's true that it's a little strange, but I'd rather continue to keep a clear separation between model definition (which determines what gets created in the database) and application usage. It's not totally implausible that someone may want to create a PostgreSQL enum type and use it from some external, non-EFCore application. I do agree that your new overload is definitely preferred for the vast majority of applications though (so it's good to have changed the docs in this way).

@austindrenski

Copy link
Copy Markdown
ContributorAuthor

@roji Good point on the external use.

Sign up for freeto 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.

3 participants

@austindrenski@roji@YohDeadfall