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

is_discrete: Rails/ThreeStateBooleanColumn: Boolean columns should always have a default value and a NOT NULL constraint #1271

Open
james-em opened this issue Mar 1, 2024 · 2 comments

Comments

@james-em
Copy link

james-em commented Mar 1, 2024

Hi,

I just discovered your library and I have been using Sidekiq for years. I'm pretty excited about what I see for now.

However, I would like to flag a Rubocop warning that I believe should be addressed.

t.boolean :is_discrete
Warning: Rails/ThreeStateBooleanColumn: Boolean columns should always have a default value and a NOT NULL constraint.

I actually do agree that boolean values should always be either FALSE or TRUE but not NULL as well.

I will be adding null: false, default: false however i'm not sure if this will be causing me troubles.

Thank you

@bensheldon
Copy link
Owner

The operational challenge of adding a default is it requires a 2-stage deploy (add a nil column, then come back and add a default)... at least until PG 11 maybe, so maybe I'm out of date on what's a safe migration.

Also, that column is a shim until the next major release of GoodJob (which is probably due about now given the number of shims).

Lastly, I can't guarantee there won't be a reasonable need for trinary columns in GoodJob in the future, so you may need to Rubocop ignore them if you are using this rule.

@james-em
Copy link
Author

james-em commented Mar 1, 2024

The operational challenge of adding a default is it requires a 2-stage deploy (add a nil column, then come back and add a default)... at least until PG 11 maybe, so maybe I'm out of date on what's a safe migration.

Also, that column is a shim until the next major release of GoodJob (which is probably due about now given the number of shims).

Lastly, I can't guarantee there won't be a reasonable need for trinary columns in GoodJob in the future, so you may need to Rubocop ignore them if you are using this rule.

Hi,

Thanks for the quick reply. I gave some testing to what you said about the 2-stage deploy.

I went ahead and spawned a Postgres 10 DB and ran the migration and it worked just fine and in a single stage the default is properly applied. (I replaced the line with t.boolean :is_discrete, null: false, default: false)

Capture d’écran, le 2024-03-01 à 11 48 01

Lastly, I can't guarantee there won't be a reasonable need for trinary columns in GoodJob in the future, so you may need to Rubocop ignore them if you are using this rule.

Does it mean that right now the NULL is value is not currently used? Maybe avoiding to use a third state should be the right thing to do also in the future? :)

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

No branches or pull requests

2 participants