Open lafriks opened 1 year ago
Should SQLite just return error as it does not support alter column?
agree
How to detect that type have changed? How to detect what constraints need to be changed?
I haven't look at postgres, but after looking at mysql doc, how about we define one method for each alter type? ex:
AlterColumnType(...)
AlterColumnConstraint(...)
* `AlterColumnType(...)` * `AlterColumnConstraint(...)`
This could probably work but still question is how to pass on to what constraint exactly needs to be changed? Create bitwise enum?
One option to create something like:
type AlterColumnOption int
const (
AlterColumnType AlterColumnOption = 1 << iota
AlterColumnDefault
AlterColumnNull
...
)
This could probably work but still question is how to pass on to what constraint exactly needs to be changed? Create bitwise enum?
oh, do you mean to pass it to adapter?
yes so that it will know what SQL to generate as currently adapter will receive not constraint options but already filled constraint struct rel.Column
that has constraints as fields but for alter it's unknown what fields need to be changed so adding such bitwise enum to rel.Column
type as field would allow to specify that
Patch coverage: 100.00%
and no project coverage change.
Comparison is base (
8750e2c
) 100.00% compared to head (2a1fe42
) 100.00%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
codeclimate test fail is not related to this code changes
For MySQL to change not null it would still require to use AlterColumType would it need to panic if not used otherwise or just do not generate SQL as for SQLite code currently does it?
Suggestions implemented
For MySQL to change not null it would still require to use AlterColumType would it need to panic if not used otherwise or just do not generate SQL as for SQLite code currently does it?
do you mean if other than AlterColumType is used? I think panic is better here so no surprise (if possible provide the suggestion in the error message)
edit: I guess just log is okay, since it's MySQL specific, panic will cause the same migration code not compatible with other db 🤔
for example: For example for Postgres you can use (but can not be supported for mysql as column full definition needs to be known):
s.AlterColumn("table", "col", rel.Require(false))
that wold generate:
alter table "table" alter column "col" drop not null;
But for mysql only default can be used this way but for not null/null change you need to use:
s.AlterColumnType("table", "col", rel.String, rel.Limit(100) rel.Require(false))
that would generate:
alter table `table` modify `col` varchar(100) null;
I see, hmm maybe let's go for panic now? and adds some comment on the alter function?
panic is not very practical and should be avoided but currently there is inconsistencies between how sqlite3 is handled (by just ignoring unsupported features) and others. Imho in the future this should be dealt with by adding error return value where it's appropriate and user should decide whether to ignore it or panic
okay, then let's align with sqlite3
updated documentation about exceptions
@lafriks is this ready to be merged?
I think yes but that depends on how you want me to proceed. Should this be merged only when all related repo changed are done? Or should this be merged and than I can create PR to other repos with correct go mod dependency update
Should this be merged only when all related repo changed are done?
I prefer this, after all merged, we can update go.mod
on separate PRs to point to tagged version before release
TODO:
For discussion:
How to detect that type have changed?How to detect what constraints need to be changed?Should SQLite just return error as it does not support alter column?