-
Notifications
You must be signed in to change notification settings - Fork 899
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
Fix ActiveRecord version check #956
Conversation
I think you're right, thanks. How about |
Ooh, I didn't realize that method existed! Yeah that's much nicer. |
Yeah, I think it was added in some version of rails 4. Hopefully rails 4.0, because we still try to support 4.0 (at least for a few more months). |
Please also add an entry to the changelog under unreleased -> fixed. Thanks. |
Hmm. Actually it looks like it wasn't added until 4.2, in this giant commit. I'm basing this on the Github history for the gem_version.rb file. Should we stick with the manual integer comparison method, then? |
Can we do something like:
We can add a TODO comment about removing the |
Merged, thanks. |
Rails 4.0 does not have the `ActiveRecord.gem_version` method, so this check will fail there. However, we know that if `gem_version` is not available, we are not dealing with Rails >= 5.0, so this will work in both cases. See paper-trail-gem#956 for an equivalent case elsewhere in the codebase.
* Fix ActiveRecord version check * Update changelog for paper-trail-gem#956 * Simplify ActiveRecord version check
Rails 4.0 does not have the `ActiveRecord.gem_version` method, so this check will fail there. However, we know that if `gem_version` is not available, we are not dealing with Rails >= 5.0, so this will work in both cases. See paper-trail-gem#956 for an equivalent case elsewhere in the codebase.
Minor change: the
RAILS_GTE_5_1
version check inrecord_trail.rb
looks wrong to me. It checks that ActiveRecord's major version is at least 5 and minor version is at least 1, but I think this check would exclude releases like 6.0 and 7.0.I've changed the check to one that will match what I think was intended.
Thanks for your hard work on this gem; it's been working great for us so far!