Efcore: Rename ForProviderXxx... methods

Created on 22 Jul 2019  路  7Comments  路  Source: dotnet/efcore

Per API review, here are the ForProviderXxx... methods so we can decide which, if any, to rename:

Cosmos

Extends class | Current | Proposal
------------ | ------------- | -------
ModelBuilder | ForCosmosHasDefaultContainer | HasDefaultContainer
EntityTypeBuilder | ForCosmosToContainer | ToContainer
EntityTypeBuilder | ForCosmosHasPartitionKey | HasPartitionKey
PropertyBuilder | ForCosmosToProperty | ToJsonProperty

SQL Server

Extends class | Current | Proposal
------------ | ------------- | -------
ModelBuilder/PropertyBuilder | ForSqlServerUseSequenceHiLo | UseHiLo
ModelBuilder/PropertyBuilder | ForSqlServerHasHiLoSequence | HasHiLoSequence
ModelBuilder/PropertyBuilder | ForSqlServerUseIdentityColumns | UseIdentityColumns / UseIdentityColumn
ModelBuilder/PropertyBuilder | ForSqlServerHasIdentitySeed | HasIdentityColumnSeed
ModelBuilder/PropertyBuilder | ForSqlServerHasIdentityIncrement | HasIdentityColumnIncrement
ModelBuilder/PropertyBuilder | ForSqlServerHasValueGenerationStrategy | HasValueGenerationStrategy
EntityTypeBuilder | ForSqlServerIsMemoryOptimized | IsMemoryOptimized
IndexBuilder | ForSqlServerIsClustered | IsClustered
IndexBuilder | ForSqlServerInclude | IncludeColumns
IndexBuilder | ForSqlServerIsCreatedOnline | IsCreatedOnline

SQLite

Extends class | Current | Proposal
------------ | ------------- | -------
PropertyBuilder | ForSqliteHasSrid | HasSrid
PropertyBuilder | ForSqliteHasDimension | HasSpatialDimension

In-memory

None

Npgsql

Extends class | Current | Proposal
------------ | ------------- | -------
ModelBuilder | ForNpgsqlHasEnum |
ModelBuilder | ForNpgsqlHasMethod |
ModelBuilder | ForNpgsqlHasRange |
ModelBuilder | ForNpgsqlUseSerialColumns |
ModelBuilder | ForNpgsqlUseIdentityAlwaysColumns |
ModelBuilder | ForNpgsqlUseIdentityByDefaultColumns |
ModelBuilder | ForNpgsqlUseIdentityColumns |
ModelBuilder | ForNpgsqlUseTablespace |
ModelBuilder/PropertyBuilder | ForNpgsqlUseSequenceHiLo |
EntityTypeBuilder | ForNpgsqlUseXminAsConcurrencyToken |
EntityTypeBuilder | ForNpgsqlHasIndex |
EntityTypeBuilder | ForNpgsqlIsUnlogged |
EntityTypeBuilder | ForNpgsqlSetStorageParameter |
EntityTypeBuilder/PropertyBuilder | ForNpgsqlHasComment |
IndexBuilder | ForNpgsqlInclude |
IndexBuilder | ForNpgsqlHasMethod |
IndexBuilder | ForNpgsqlHasOperators |
IndexBuilder | ForNpgsqlHasCollation |
IndexBuilder | ForNpgsqlHasSortOrder |
IndexBuilder | ForNpgsqlHasNullSortOrder |

closed-fixed type-enhancement

Most helpful comment

I know we go around in circles on this, but I'm starting to question the value of having the provider name in there at all. A lot of concepts are pretty unique to the provider: Identity columns, memory-optimized tables, clustered indexes, included index properties, etc.

Maybe it would be better to optimize the APIs for the single-provider case and warn when the active provider isn't what we expect.

All 7 comments

Of course, Npgsql is the over-achiever again. ;-)

Some thoughts/observations:

  • Is HasCosmosProperty meant to be ToCosmosProperty?
  • ForXyzInclude vs XyzInclude on IndexBuilder: Somehow ForXyzInclude still seems to make more sense. Perhaps it is because there is not "is", "has", or "use" verb?
  • Does Npgsql need to keep ForNpgsqlHasComment?

I know we go around in circles on this, but I'm starting to question the value of having the provider name in there at all. A lot of concepts are pretty unique to the provider: Identity columns, memory-optimized tables, clustered indexes, included index properties, etc.

Maybe it would be better to optimize the APIs for the single-provider case and warn when the active provider isn't what we expect.

I agree with most of what @divega and @bricelam said, but after sleeping on this and looking at it again I wonder if there is really enough value to make a breaking change here. It will make code prettier, shorter, etc., but I'm not sure it makes it easier to understand, and so maybe we should just leave what we have as is.

Does Npgsql need to keep ForNpgsqlHasComment?

Nope... As soon as I finish the preview7 port I'll also switch the comment support to use relational. It'll be a breaking change but it's only comments...

FWIW I think I'm leaning towards what @ajcvickers is saying. I agree we should focus on the single-provider case, but it simply doesn't seem important enough to do a breaking change...

Decisions:

  • Obsolete and rename for 3.0
  • Look at sponsor classes to make sure there are provider differences
  • Go for no-provider in the method names
Was this page helpful?
0 / 5 - 0 ratings