Add method to alter table column

[lafriks]

TODO:

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

[Fs02]

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

agree

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

[lafriks]

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

[lafriks]

One option to create something like:

type AlterColumnOption int

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

[Fs02]

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?

[lafriks]

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

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

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

Additional details and impacted files

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

Files Changed	Coverage Δ
column.go	100.00% <100.00%> (ø)
query.go	100.00% <100.00%> (ø)
schema.go	100.00% <100.00%> (ø)
schema_options.go	100.00% <100.00%> (ø)
table.go	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lafriks]

codeclimate test fail is not related to this code changes

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

[lafriks]

Suggestions implemented

[Fs02]

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 🤔

[Fs02]

LGTM, Thanks

[lafriks]

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;

[Fs02]

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

[lafriks]

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

[Fs02]

okay, then let's align with sqlite3

[lafriks]

updated documentation about exceptions

[Fs02]

@lafriks is this ready to be merged?

[lafriks]

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

[Fs02]

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