Skip to content
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

Merged
merged 3 commits into from
Apr 26, 2017

Conversation

newtrat
Copy link
Contributor

@newtrat newtrat commented Apr 26, 2017

Minor change: the RAILS_GTE_5_1 version check in record_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!

@jaredbeck
Copy link
Member

.. I think this check would exclude releases like 6.0 and 7.0.

I think you're right, thanks.

How about ::ActiveRecord.gem_version >= ::Gem::Version.new("5.1.0.beta1")?

@newtrat
Copy link
Contributor Author

newtrat commented Apr 26, 2017

Ooh, I didn't realize that method existed! Yeah that's much nicer.

@jaredbeck
Copy link
Member

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).

@jaredbeck
Copy link
Member

Please also add an entry to the changelog under unreleased -> fixed. Thanks.

@newtrat
Copy link
Contributor Author

newtrat commented Apr 26, 2017

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?

@jaredbeck
Copy link
Member

jaredbeck commented Apr 26, 2017

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:

::ActiveRecord.respond_to?(:gem_version) && 
  ::ActiveRecord.gem_version >= ::Gem::Version.new("5.1.0.beta1")

We can add a TODO comment about removing the respond_to? when we drop support for rails 4.0 (soon!).

@jaredbeck jaredbeck merged commit b0b201a into paper-trail-gem:master Apr 26, 2017
@jaredbeck
Copy link
Member

Merged, thanks.

magnusvk added a commit to magnusvk/paper_trail that referenced this pull request Sep 5, 2017
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.
aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020
* Fix ActiveRecord version check

* Update changelog for paper-trail-gem#956

* Simplify ActiveRecord version check
aried3r pushed a commit to aried3r/paper_trail that referenced this pull request Dec 14, 2020
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants