Skip to content
This repository has been archived by the owner on Jul 5, 2023. It is now read-only.

Make belongs_to relation non-nilable by default #144

Merged
merged 5 commits into from
Sep 4, 2019

Conversation

wpride
Copy link
Contributor

@wpride wpride commented Aug 28, 2019

Since Rails 5 the belongs_to relation has been required by default. To make this non-required you must add optional: true to the relation.

This change updates the association methods to reflect this.

Curious what you all think about this change; if it looks good I will add tests.

@codecov-io
Copy link

codecov-io commented Aug 28, 2019

Codecov Report

Merging #144 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   98.36%   98.36%   +<.01%     
==========================================
  Files         169      169              
  Lines        1830     1832       +2     
==========================================
+ Hits         1800     1802       +2     
  Misses         30       30
Impacted Files Coverage Δ
.../sorbet-rails/model_plugins/active_record_assoc.rb 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec48634...00d024a. Read the comment docs.

@hdoan741
Copy link
Contributor

Sounds good. I verified that this is indeed the behavior in Rails 5.
I'm curious though: what happen if you have a belongs_to, but the column is null? Is it just an ActiveRecord level check?

https://blog.bigbinary.com/2016/02/15/rails-5-makes-belong-to-association-required-by-default.html

@wpride
Copy link
Contributor Author

wpride commented Sep 4, 2019

@manhhung741 Yes, looks like you get an ActiveRecord::NotNullViolation:

Updated expected results

@hdoan741 hdoan741 merged commit fa8d28b into chanzuckerberg:master Sep 4, 2019
@hdoan741
Copy link
Contributor

hdoan741 commented Sep 4, 2019

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants