Add three-state boolean problem #275
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bbatsov
reviewed
Nov 24, 2020
bbatsov
reviewed
Nov 24, 2020
pirj
reviewed
Nov 24, 2020
tejasbubane
force-pushed
the
three-state-boolean
branch
from
a3397ca
to
ab800ff
Compare
pirj
approved these changes
Dec 2, 2020
tejasbubane
force-pushed
the
three-state-boolean
branch
from
ab800ff
to
c002434
Compare
|
Thanks @pirj I have made the suggested changes. |
pirj
reviewed
Dec 3, 2020
tejasbubane
force-pushed
the
three-state-boolean
branch
from
c002434
to
371d044
Compare
andyw8
reviewed
Dec 3, 2020
README.adoc
Outdated
| With SQL databases, if a boolean column is not given a default value, it ends up having three possible values: `true`, `false` and `NULL`. | ||
| Boolean operators [work in unexpected ways](https://en.wikipedia.org/wiki/Three-valued_logic) with `NULL`. | ||
|
|
||
| E.g. in SQL queries, `true AND NULL` is `NULL` (not false), `true AND NULL OR false` is `NULL` (not false). This can make SQL queries return unexpected results. |
There was a problem hiding this comment.
Minor, but starting a sentence with "E.g." feels awkward. How about "For example"?
andyw8
reviewed
Dec 3, 2020
README.adoc
Outdated
| @@ -1026,6 +1026,24 @@ end | |||
| While enforcing table defaults only in Rails is suggested by many Rails developers, it's an extremely brittle approach that leaves your data vulnerable to many application bugs. | |||
| And you'll have to consider the fact that most non-trivial apps share a database with other applications, so imposing data integrity from the Rails app is impossible. | |||
|
|
|||
| === 3-state Boolean [[three-state-boolean]] | |||
|
|
|||
| With SQL databases, if a boolean column is not given a default value, it ends up having three possible values: `true`, `false` and `NULL`. | |||
There was a problem hiding this comment.
I think "will have" reads better than "ends up having".
andyw8
reviewed
Dec 3, 2020
README.adoc
Outdated
|
|
||
| E.g. in SQL queries, `true AND NULL` is `NULL` (not false), `true AND NULL OR false` is `NULL` (not false). This can make SQL queries return unexpected results. | ||
|
|
||
| To avoid such situations, boolean columns should always have a default value and `NOT NULL` constraint. |
andyw8
approved these changes
Dec 3, 2020
koic
reviewed
Dec 4, 2020
| add_column :users, :active, :boolean | ||
|
|
||
| # good - boolean with a default value and with restricted NULL | ||
| add_column :users, :active, :boolean, default: true, null: false |
There was a problem hiding this comment.
I'd prefer an example code of the rule that defaults to false here :-) Of course, In practice, false or true is chosen depending on the context.
There was a problem hiding this comment.
Or I think one approach is to exemplify an example with default: false and an example with default: true.
koic
reviewed
Dec 4, 2020
README.adoc
Outdated
| # bad - boolean without a default value | ||
| add_column :users, :active, :boolean | ||
|
|
||
| # good - boolean with a default value and with restricted NULL |
There was a problem hiding this comment.
Suggested change
| # good - boolean with a default value and with restricted NULL | |
| # good - boolean with a default value (`false` or `true`) and with restricted NULL |
tejasbubane
force-pushed
the
three-state-boolean
branch
from
371d044
to
b329f73
Compare
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: rubocop/rubocop-rails#337
Ref: https://thoughtbot.com/blog/avoid-the-threestate-boolean-problem