Skip to content
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

Alter nullable constraint when alterNullable is set to true #4730

Merged
merged 4 commits into from Jan 24, 2022
Merged

Alter nullable constraint when alterNullable is set to true #4730

merged 4 commits into from Jan 24, 2022

Conversation

yhaskell
Copy link
Contributor

@yhaskell yhaskell commented Oct 13, 2021

As discussed in the #4401 Issue, I created a PR to make it so nullable alterations only apply when ({ alterNullable: true }) is provided as a param to .alter() function (which is a default value, since we do not want to break the previous behaviour).

@elhigu please help further :)

@yhaskell yhaskell marked this pull request as draft October 13, 2021 06:28
@kibertoad
Copy link
Collaborator

Can you also add an integration test? there are few dealing with nullability in integration2, you can build from there

@cpaczek
Copy link

cpaczek commented Jan 7, 2022

This is also an issue on CockroachDB. See: strapi/strapi#11738

@kibertoad
Copy link
Collaborator

@OlivierCavadenti this might also look good in 1.0.0

@OlivierCavadenti OlivierCavadenti self-assigned this Jan 8, 2022
@intech
Copy link
Contributor

intech commented Jan 8, 2022

@OlivierCavadenti before doing anything, check out this post strapi/strapi#11738 (comment)

@ethan-gallant
Copy link

Can we get this merged? It's causing quite a headache for various projects depending on knex.

@kibertoad
Copy link
Collaborator

@ethan-gallant tests are missing and there are conflicts. wiuld you like to help?

@OlivierCavadenti
Copy link
Collaborator

OlivierCavadenti commented Jan 24, 2022

@OlivierCavadenti before doing anything, check out this post strapi/strapi#11738 (comment)

I can check tomorrow I think. I need to reread the thread to understand the problem ^^.

@OlivierCavadenti OlivierCavadenti marked this pull request as ready for review January 24, 2022 19:21
@OlivierCavadenti OlivierCavadenti merged commit 3ee6d0f into knex:master Jan 24, 2022
@kibertoad
Copy link
Collaborator

@OlivierCavadenti Do you think we can publish next semver minor now?

@intech
Copy link
Contributor

intech commented Jan 24, 2022

@OlivierCavadenti as it turned out, this is irrelevant, there is a problem with the requests themselves, just a similar error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants