Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename ForProviderXxx... methods #16686

Closed
ajcvickers opened this issue Jul 21, 2019 · 7 comments · Fixed by #16735
Closed

Rename ForProviderXxx... methods #16686

ajcvickers opened this issue Jul 21, 2019 · 7 comments · Fixed by #16735
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Contributor

ajcvickers commented Jul 21, 2019

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
@ajcvickers
Copy link
Contributor Author

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

@divega
Copy link
Contributor

divega commented Jul 22, 2019

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?

@bricelam
Copy link
Contributor

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.

@ajcvickers
Copy link
Contributor Author

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.

@roji
Copy link
Member

roji commented Jul 22, 2019

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...

@ajcvickers
Copy link
Contributor Author

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

@roji
Copy link
Member

roji commented Jul 26, 2019

Matching PR for EFCore.PG: npgsql/efcore.pg#948

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants