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

Make genericSerial generic over Integral #534

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thomasjm
Copy link
Contributor

As requested on #532

Copy link
Contributor

@3noch 3noch left a comment

Choose a reason for hiding this comment

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

cc @kmicklas - in case you can see effects of this change that I cannot.

@@ -183,7 +183,7 @@ instance FieldReturnType 'True 'False Sqlite resTy a =>
field' _ _ nm ty _ collation constraints SqliteHasDefault =
field' (Proxy @'True) (Proxy @'False) nm ty Nothing collation constraints

instance BeamSqlBackendHasSerial Sqlite where
instance (Integral n) => BeamSqlBackendHasSerial n Sqlite where
Copy link
Member

Choose a reason for hiding this comment

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

This should have a IsSqliteSerialIntegerType constraint.

@@ -452,5 +452,5 @@ instance Db.FieldReturnType 'True 'False Postgres resTy a =>
field' _ _ nm ty _ collation constraints PgHasDefault =
Db.field' (Proxy @'True) (Proxy @'False) nm ty Nothing collation constraints

instance BeamSqlBackendHasSerial Postgres where
instance (Integral n) => BeamSqlBackendHasSerial n Postgres where
Copy link
Member

Choose a reason for hiding this comment

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

This instance should be split for Int16, Int32, and Int64 and use smallserial, serial, and bigserial, respectively.

BeamSqlBackendHasSerial be where
genericSerial :: FieldReturnType 'True 'False be (SqlSerial Int) a => Text -> a
class (BeamMigrateSqlBackend be, Integral n) =>
BeamSqlBackendHasSerial n be where
Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favor of removing the Integral constraint here. Integral implies Num which implies the ability to do arithmetic. However, I can imagine wanting to add support for an opaque ID type which prohibits arithmetic. Backends already have the ability to constrain the valid ns in their instances.

@kmicklas kmicklas added the breaking change Change will require a major version bump label Jan 10, 2021
@kmicklas kmicklas added this to the 0.10.0.0 milestone Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Change will require a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants