rel

rel copied to clipboard

TODO:

• [x] Way to detect what constraints are applied

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?~~

Nov 01 '22 10:11 lafriks

Should SQLite just return error as it does not support alter column?

agree

Nov 02 '22 01:11 Fs02

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

Nov 06 '22 00:11 Fs02

* `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?

Nov 07 '22 11:11 lafriks

One option to create something like:

type AlterColumnOption int

const (
  AlterColumnType AlterColumnOption = 1 << iota
  AlterColumnDefault
  AlterColumnNull
  ...
)

Nov 07 '22 11:11 lafriks

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?

Nov 08 '22 00:11 Fs02

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

Nov 08 '22 09:11 lafriks

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8750e2c) 100.00% compared to head (2a1fe42) 100.00%.

@@            Coverage Diff            @@
##            master      #324   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           35        35           
  Lines         2826      2859   +33     
=========================================
+ Hits          2826      2859   +33     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Nov 08 '22 15:11 codecov[bot]

codeclimate test fail is not related to this code changes

Nov 08 '22 23:11 lafriks

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?

Nov 09 '22 09:11 lafriks

Suggestions implemented

Nov 09 '22 09:11 lafriks

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 🤔

Nov 10 '22 00:11 Fs02

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;

Nov 10 '22 00:11 lafriks

I see, hmm maybe let's go for panic now? and adds some comment on the alter function?

Nov 10 '22 01:11 Fs02

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

Nov 10 '22 08:11 lafriks

okay, then let's align with sqlite3

Nov 10 '22 08:11 Fs02

updated documentation about exceptions

Nov 10 '22 13:11 lafriks

@lafriks is this ready to be merged?

Nov 15 '22 13:11 Fs02

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

Nov 15 '22 13:11 lafriks

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

Nov 16 '22 01:11 Fs02