Add alterType and update index.d.ts for alter function #4967
Conversation
OlivierCavadenti
commented
Jan 25, 2022
•
OlivierCavadenti
commented
- Fix https://github.com/knex/knex/pull/4730/files#r791157891
- And I think we are good for an 1.0.2 (doc here : Add alterNullable doc documentation#385)
|
@intech do you think I can do something for cockroachdb test ? Or I need to revert for now ? |
Quickly studied the error and can not say how to get around it. I'll ask the cockroachdb engineers for advice and get back. |
|
@OlivierCavadenti This column change is not yet supported cockroachdb/cockroach#47636 |
|
wait, wasn't the whole idea of the change to avoid altering nullability? or crdb doesn't support such change even without toucing nullability? |
|
@kibertoad I don't really understand the real application of this functionality, which I wrote about in the original issue on another project using knex. alter table "primary_table_null" alter column "id_not_nullable_primary" type integer using ("id_not_nullable_primary"::integer);Why do we need to change the column type to int if we already have int when creating the table? create table "primary_table_null" ("id_not_nullable_primary" integer not null);@OlivierCavadenti @kibertoad Could you please explain to me the application of these queries? |
|
@intech I think what happens there is that since knex does not know what is the current type of the column, it uses the one specified in alter command. To address that we probably need additional parameter alterType, which omits type change if set. |
OlivierCavadenti
changed the title
| @@ -2210,7 +2210,7 @@ export declare namespace Knex { | |||
| notNullable(): ColumnBuilder; | |||
| nullable(): ColumnBuilder; | |||
| comment(value: string): ColumnBuilder; | |||
| alter(): ColumnBuilder; | |||
| alter(options: Readonly<{alterNullable?: boolean, alterType?: boolean}>): ColumnBuilder; | |||
There was a problem hiding this comment.
this is breaking for TS users. the parameter was not there, now its required.
There was a problem hiding this comment.
Yeah, sorry, just fixed and merged here : #4996
There was a problem hiding this comment.
will publish hotfix shortly
