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

Add change nullability statement #1041

Merged

Conversation

davidepaolotua
Copy link
Contributor

This PR attempts to fix two things:

provide a way to change the nullability of a column in the migrator.

Related to #1039
Two new macros have been created:

  • allow_nulls_for :column, which allows for the :column to receive null values
  • forbid_nulls for :column, which set the not null constraint
    it is split from the change_type statement, so that you can theoretically do something like
change_type the_column : String
# migrate somehow the null fields present in the table
forbid_nulls_for :the_column

changes the order of the change_default statement with respect to the change_type statement

Currently, if you had something like:

change_type a : String
change_default a : String, default: "Whatever"

it would generate first the change of default, then the change of type, which is probably counterintuitive.
Now, it generates first the change of type, then the change of default

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Let's give this a shot 🚀

@jwoertink jwoertink merged commit 1326a8e into luckyframework:main May 16, 2024
8 of 9 checks passed
jwoertink pushed a commit that referenced this pull request May 17, 2024
* Implement allow_nulls_for and forbid_nulls_for

* Remove leftover macro, fix ordering of change default with respect to change type

* Implement have_any? criteria

* Add change nullability statement (#1041)

* Implement allow_nulls_for and forbid_nulls_for

* Remove leftover macro, fix ordering of change default with respect to change type

* Prevent users from assuming boolean result

* Implement allow_nulls_for and forbid_nulls_for

* Remove leftover macro, fix ordering of change default with respect to change type

* Implement have_any? criteria

* Prevent users from assuming boolean result
@davidepaolotua davidepaolotua deleted the add-change-nullability-statement branch May 21, 2024 08:54
@jwoertink
Copy link
Member

@davidepaolotua I was just writing up some documentation and it just occurred to me.... why did we add allow_nulls_for and forbid_nulls when we have make_required and make_optional which generate the same SQL? 🤔

def build
if @required
change = "SET"
else
change = "DROP"
end
String.build do |index|
index << "ALTER TABLE #{@table}"
index << " ALTER COLUMN #{@column}"
index << " #{change} NOT NULL;"
end

def build_nullability_statement(column_name, nullability)
"ALTER TABLE #{@table_name} ALTER COLUMN #{column_name} #{nullability ? "DROP" : "SET"} NOT NULL;"
end

I'm sure there was some logic behind having a second way to do this, but I'm not seeing why, and I don't remember 😅

@davidepaolotua
Copy link
Contributor Author

Uhm... I wish I was as sure as you are...

imho I just completely missed the existence of that method - I prolly just looked in the alter_table file, and didn't occur to me that there was actually a separated file only for that case, while the other change_xxx are in alter table ^^'

we actually could remove those methods imho (just keeping the fix for the ordering of the ddl statements)

@jwoertink
Copy link
Member

Ok. I'll at least open an issue on it. I'm sure I just forgot about the other methods too at the time 😂

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.

2 participants