Skip to content

Suggestion: 3-state booleans don't require a default #343

Open
@joerixaop

Description

@joerixaop

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).

Activity

pirj

pirj commented on Aug 29, 2023

@pirj
Member

Have you had a chance to check #275 #337 #43?

joerixaop

joerixaop commented on Aug 30, 2023

@joerixaop
Author

#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).

pirj

pirj commented on Sep 2, 2023

@pirj
Member

If you would decide to send a PR, what would it add/remove/change?

armandmgt

armandmgt commented on Jan 13, 2025

@armandmgt

Jumping in because I recently faced this:
We could enforce the default only when using add_column or change_table. When creating a new table only the nullability should be checked.

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 ?

pirj

pirj commented on Jan 13, 2025

@pirj
Member

We could enforce the default only when using add_column or change_table. When creating a new table only the nullability should be checked.

I didn't get, why we can only enforce the default when changing an existing table. Can you please explain, @armandmgt ?

armandmgt

armandmgt commented on Jan 13, 2025

@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_exemptiontax_exemption can be equally likely true or false)

pirj

pirj commented on Jan 13, 2025

@pirj
Member

tax_exemption is either true or false, it shouldn’t be null, or can it?

But now I see what you mean.
The point is indeed not to enforce the presence of a default value, the point is to prevent nulls.
Is this what you had in mind?

armandmgt

armandmgt commented on Jan 14, 2025

@armandmgt

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

pirj

pirj commented on Jan 14, 2025

@pirj
Member

@koic @andyw8 Without diving too fare into the discussion, can you please check the last few messages.
Does the suggestion to change the 3-state boolean guideline to have emphasis on being NOT NULL instead of enforcing a default value sound like a reasonable change to you?

cc @tejasbubane just in case, this was your original contribution to the style guide.

koic

koic commented on Jan 14, 2025

@koic
Member

Yeah, the sequence of first setting null: false and then specifying a default value of either true or false seems appropriate, IMO.

In other words, focusing on preventing three-state values by ensuring that boolean columns have null: false as the top priority makes sense.

pirj

pirj commented on Jan 14, 2025

@pirj
Member

@armandmgt would you like to send a PR?

armandmgt

armandmgt commented on Jan 14, 2025

@armandmgt

@pirj Yes ! I opened one here #363 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @pirj@koic@joerixaop@armandmgt

      Issue actions

        Suggestion: 3-state booleans don't require a default · Issue #343 · rubocop/rails-style-guide