Fixed Calling << to an ActiveModel::Errors deprecation warning
#690
Conversation
|
This is currently failing linting due to:
Any suggestion on how to resolve that @lcreid ? |
There was a problem hiding this comment.
Thanks for submitting a PR. We should have a test that shows that this is fixed. I believe our test suite wasn't triggering this code before, which is why we never noticed the error. Do you have a test that can reproduce the error (before your fix) that we could add to our tests?
I wonder why that's come up for you? Perhaps that's why the original code used a temporary variable? For now, put |
|
@lcreid ive added a test case which triggers the warning when you change |
There was a problem hiding this comment.
Do we need the AddressesController and the routes for addresses?
| object.class.try(:reflections)&.each do |association_name, a| | ||
| next unless a.is_a?(ActiveRecord::Reflection::BelongsToReflection) | ||
| next unless a.foreign_key == name.to_s | ||
|
|
||
| messages << object.errors[association_name] | ||
| object.errors[association_name].each do |error| | ||
| object.errors.add(name, error) |
There was a problem hiding this comment.
I was intrigued by your comment that it didn't say anything at version 7. You would think it should have errored, if the deprecation warning was in 6.1. You're right. << seems to still work, and without a deprecation message. Very strange.
I was unable to get it to show me the deprecation message, even with version 6.1. (I tweaked the code to run your test case, but with the << operator instead of #add.) Also strange. At any time, did you see the deprecation warning when running the test you added?
There was a problem hiding this comment.
If i revert my changes to get_error_messages but leave the new testcase in place i get the following:
|
Oops. Looks like the test fails on edge. |
It looks like it doesn't pick up that the relationship is required in rails 7.1. Possibly something in |
Fixes #689