Turn off enforcement of not-null db validations requiring default values

[apalmer0]

# bad
  def change
    add_column :table_name, :column_name, :string, null: false, default: "foo"
  end

# good
  def change
    add_column :table_name, :column_name, :string, null: false
  end

(not so great) documentation

I don't know if people agree with this but I think I'd rather a save fail if a value is null rather than succeed by saving some arbitrary default value.

[danfrenette]

This is something I'd definitely like to see discussion on. I can definitely see a case (which should be avoided at all costs) where someone has to write a crazy data migration rake task because records, where the default value was intended vs it's there just because it's the default, are totally indistinguishable.

Default values are super useful if the business logic of an app necessitates one, and the default value itself is reasonable, but I'm not so sure about it being required. Would love to hear more thoughts on this though.

[apalmer0]

Yeah of the PRs I have open now I think this one should get the most discussion. I'm not sold on this stance but I think it's worth considering... my thought has been that business logic constraints should live in the model, not the db, so if you do need a default value you should set it there instead.

[kevin-j-m]

I can go either way on what we suggest for enforcement here. I can see both sides, but for some thoughts:

• This is a suggestion that I always view with a critical eye. There are definitely scenarios where I don't want a field to be null, but I also don't want to suggest a default because there is no sane default.
• I'm also thankful when I do miss including one to have the prodding to ask myself if there is a default that makes sense. Could that lead to putting one in when it doesn't make sense to? Yes, I suppose, but I could still make that mistake at the model layer or anywhere else too.
• Databases are pretty good at handling defaults themselves, so to the extent that it makes sense, it can provide benefits. It does de-couple from the rest of your business logic, but it is, inherently, part of your system still.

[mikestone14]

I'm in favor of this change. I like the Rubocop cops that should always be part of our code style. In this case, since there are valid situations where you would not want a default value I don't think we should have Rubocop insist that one should be provided. That could in turn influence an engineer to provide a default value when one shouldn't be there which can have unintended consequences and would be difficult for a reviewer without a ton of context to catch.

[kevin-j-m]

I like the Rubocop cops that should always be part of our code style.

If that's all we want to use, then we should probably turn almost every setting off, because valid exceptions run abound.

[mikestone14]

If that's all we want to use, then we should probably turn almost every setting off, because valid exceptions run abound.

Maybe there's a misunderstanding between always part of our code style and always adhered to. For example, I like the code style regarding line length. However, I find that it isn't always a good idea to adhere to this style, especially in test files. Maybe that cop can be configured to ignore test files but that's another story. As it is, I think there's value to Rubocop pointing out when lines go beyond a certain length. I don't feel the same way about this cop because in some scenarios adding a default value is wrong.

[kevin-j-m]

@apalmer0 can you rebase this so we can merge it in?