-
Notifications
You must be signed in to change notification settings - Fork 170
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 n
s in their instances.
As requested on #532