-
-
Notifications
You must be signed in to change notification settings - Fork 912
Introduce support for deprecated associations API #1690
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
base: main
Are you sure you want to change the base?
Introduce support for deprecated associations API #1690
Conversation
spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb
Outdated
Show resolved
Hide resolved
| end | ||
|
|
||
| context 'have_many' do | ||
| if rails_version <= 8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to test the error message, but I can remove this context if it's too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, the only missing thing is that the deprecated association option will not exist in older rails versions, so you should only use the deprecated modifier method in the test.
expect do
expect(having_many_children).to have_many(:children).deprecated(false)
end.to raise_error(NotImplementedError, expected_message)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Fixed it in 9450ae8
Rails 8.1 introduced the ability to mark Active Record
associations as deprecated. This allows
developers to mark associations for deprecation while maintaining
backward compatibility.
Assert association is deprecated:
```ruby
it { should have_many(:posts).deprecated }
it { should have_one(:profile).deprecated }
it { should belong_to(:author).deprecated }
```
Assert association is NOT deprecated:
```ruby
it { should have_many(:posts).deprecated(false) }
```
d6ed0e5 to
70d2cc8
Compare
| # ##### deprecated | ||
| # | ||
| # Use `deprecated` to assert that the `:deprecated` option was specified. | ||
| # (Enabled by default in Rails 8.1+). | ||
| # | ||
| # class Account < ActiveRecord::Base | ||
| # belongs_to :bank, deprecated: true | ||
| # end | ||
| # | ||
| # # RSpec | ||
| # RSpec.describe Account, type: :model do | ||
| # it { should belong_to(:bank).deprecated(true) } | ||
| # end | ||
| # | ||
| # # Minitest (Shoulda) | ||
| # class AccountTest < ActiveSupport::TestCase | ||
| # should belong_to(:bank).deprecated(true) | ||
| # end | ||
| # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this up before the line 375
# ... new doc
#
# @return [AssociationMatcher]The same comment applies to the other doc comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks! Done in 2239770
| if ::ActiveRecord::VERSION::STRING >= '8.1' | ||
| @options[:deprecated] = deprecated | ||
| self | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have a new method in the lib/shoulda/matchers/rails_shim.rb file to have that active record version check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! Refactored this in 9450ae8
| submatchers_match? && | ||
| deprecated_correct? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave the submatchers_match? as the last check, as they're usually a slower check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in df61574
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need tests that assert that the correct message output, e.g.
it 'rejects an association with a non-matching :autosave option with the correct message' do
message = 'Expected Vehicle to have a belongs_to association called drivable (drivable should have autosave set to true)'
expect {
expect(delegating_type_to_drivable(autosave: false)).to have_delegated_type(:drivable).autosave(true)
}.to fail_with_message(message)
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another great catch. I missed this test 🙈 Added these in 4ea3d9c
|
thanks @matsales28 for the review! I will come back to this tomorrow. Had some issues locally after pushing the branch and need to figure it out to fix the test. |
Let's use the Shoulda::Matchers::RailsShim class to encapsulate this logic. I also updated the unit tests to have an active_record_version helper.
147e89a to
bcba32b
Compare
Closes #1681
Rails 8.1 introduced the ability to mark Active Record associations as deprecated. This allows developers to mark associations for deprecation while maintaining backward compatibility.
Example test cases
Assert association is deprecated:
Assert association is NOT deprecated:
Notes and Questions
NotImplementedErrormakes sense to me, although I debated on returningArgumentError, since that's what ActiveRecord returns when trying to use it on versions < 8.1. Open to suggestions to provide more helpful error on olderAR versions.
shoulda-matcherscan be used in a non-Rails app.Tests with a real app
I tested the code changes with these scripts: https://gist.github.com/stefannibrasil/02809758148838bf3950d34a64c37106 (Rails 8.0 and 8.1.1). Both provide lots of examples of how to use the new matcher.