Fix deprecation introduced in ActiveRecord 5.1.0.beta1. Closes #257 #259
Conversation
| @@ -393,7 +393,13 @@ def insert_at_position(position) | |||
| end | |||
|
|
|||
| def update_positions | |||
| old_position = send("#{position_column}_was") || bottom_position_in_list + 1 | |||
| if ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1 | |||
There was a problem hiding this comment.
This should be encapsulated in a method.
There was a problem hiding this comment.
ActiveRecord version check? Something along those lines?
def activerecord_version_at_least?(major, minor = nil)
ActiveRecord::VERSION::MAJOR >= major && (!minor || ActiveRecord::VERSION::MINOR >= minor)
end
# activerecord_version_at_least?(5, 1)Or is that way of specifying the version a bit awkward and it would be better to go with version strings?
def activerecord_version_at_least?(required)
Gem::Version.new(ActiveRecord::VERSION::STRING) >= Gem::Version.new(required)
end
# activerecord_version_at_least?('5.1')There was a problem hiding this comment.
In fact, let's get rid of checking the version altogether, and rely on respond_to? for this. Why check version, When we can duck type.
There was a problem hiding this comment.
I can do either way, but @brendon made a (valid) suggestion that version comparison makes it easier to remove old code path in the future.
That's what's currently done in:
https://github.com/swanandp/acts_as_list/blob/f9b40e1cbdefe5244b787c66da74148e351af26c/lib/acts_as_list/active_record/acts/sequential_updates_method_definer.rb#L8
(which could be replaced w/ eventual activerecord_version_at_least?('5')).
There was a problem hiding this comment.
Makes sense. Let's just leave it at a direct comparison for now, and we can come back to it later. So no change needed here, just the other minor one and this is good to go.
| if ActiveRecord::VERSION::MAJOR >= 5 && ActiveRecord::VERSION::MINOR >= 1 | ||
| old_position = send("#{position_column}_before_last_save") | ||
| else | ||
| old_position = send("#{position_column}_was") |
There was a problem hiding this comment.
old_position = can be outside the if clause for better readability.
There was a problem hiding this comment.
Extracted that block to a method. It's clean and leaves original method basically intact.
`attribute_was` replaced by `attribute_before_last_save`
CvX
force-pushed
the
rails-5.1.0.beta1
branch
from
36969ac to
64b6e15
Compare
|
Merged. Thanks @CvX ! |
|
Hi, have to dig up this PR because I've noticed that the code I committed and which ended up in the repository is incorrect. We can either correct the conditional ( Please let me know which way do you prefer and I'll submit a PR. |
|
Thanks @CvX, I've pushed a fix for that to master :) Thanks for picking it up! :) |
Added ruby 2.4 and Rails 5.1.0.beta1 to test matrix. All tests are passing. Also, the change was tested in a live application.
Thanks to @brendon for guidance! 😃