-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Suggestion: 3-state booleans don't require a default #343
Comments
#43 is about where to add a default if you want to add one. I don't want to add one, but I do think a lint about booleans not being nullable is sensible, adding a requirement for a default value in that same lint makes it difficult to use when you do not want a default value . #275 doesn't seem to have any discussion about why requiring a default would be part of the lint that requires a boolean to be not-null. The linked blog post points out what I also pointed out: a default is required when adding a column to an existing table with existing rows, that is guaranteed to be not the case when you are adding a boolean column with create_table. I don't really understand what #337 has to do with default values (boolean or otherwise). |
If you would decide to send a PR, what would it add/remove/change? |
Jumping in because I recently faced this: In terms of good/bad examples we could make the following change: - # bad - boolean without a default value
+ # bad - boolean without restricted `NULL`
add_column :users, :active, :boolean
+ t.boolean :users, :active, :boolean
- # good - boolean with a default value (`false` or `true`) and with restricted `NULL`
+ # good - boolean with restricted `NULL` and with a default value (`false` or `true`) for existing tables
add_column :users, :active, :boolean, default: true, null: false
add_column :users, :admin, :boolean, default: false, null: false
+ t.boolean :users, :active, :boolean, null: false Would that make sense @pirj ? |
I didn't get, why we can only enforce the default when changing an existing table. Can you please explain, @armandmgt ? |
In the thoughtbot article used as reference for this cop, they suggest to add the default because otherwise the migration would fail for existing rows. When creating a new table the default value is not necessary, and sometimes it does not make sense (e.g. a Payments table with a column |
tax_exemption is either true or false, it shouldn’t be null, or can it? But now I see what you mean. |
Exactly, the tax_exemption column would still have the NOT NULL constraint 👍 And yes, my interpretation of the thoughtbot article is that we should enforce the NOT NULL constraint. The default values are a secondary matter, relevant only for existing tables |
@koic @andyw8 Without diving too fare into the discussion, can you please check the last few messages. cc @tejasbubane just in case, this was your original contribution to the style guide. |
Yeah, the sequence of first setting In other words, focusing on preventing three-state values by ensuring that |
@armandmgt would you like to send a PR? |
I'm not quite sure why it's necessary for booleans to have a default value. Obviously when you are adding a boolean column to an existing table it may be required, because adding non-null columns requires a default in most cases. A newly created table however doesn't have a technical need for a default value for non-null tables, and it seems entirely plausible for a column not to have a good default value (whether it's an integer/string or as in this case a boolean).
The text was updated successfully, but these errors were encountered: