-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add alterType and update index.d.ts for alter function #4967
Conversation
OlivierCavadenti
commented
Jan 25, 2022
•
edited
Loading
edited
- 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. |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is breaking for TS users. the parameter was not there, now its required.
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.
Yeah, sorry, just fixed and merged here : #4996
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.
will publish hotfix shortly